DynamoDB: Check for children more efficiently #2722
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TobiasBales
approved these changes
May 12, 2017
physical/dynamodb.go
Outdated
| @@ -378,6 +378,39 @@ func (d *DynamoDBBackend) List(prefix string) ([]string, error) { | |||
| return keys, nil | |||
| } | |||
|
|
|||
| // hasChildren returns true if there exist items below a certain path prefix. | |||
| // To do so, the method fetches such items from DynamoDB. If there are more than one item (which is the "directory" | |||
| // item), there are children. | |||
There was a problem hiding this comment.
Small nitpick, but could you wrap comments to a width of 80?
physical/dynamodb.go
Outdated
| }, | ||
| }, | ||
| // Avoid fetching too many items from DynamoDB for performance reasons. | ||
| // We need at least two because one is the directory item, all others are children. |
|
Same comment as brian -- looks good after the comment nitpicking :-) |
|
Sure, no problem! Done 😉 |
briankassouf
approved these changes
May 15, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When deleting items from Vault, the DynamoDB backend checks whether there are any more items in the same path and cleans it up if not. This works pretty well for nodes with only a few children. In our infrastructure however, we experience issues with the revocation of leases that reside in
auth/github/login/.Below this node, there are so many items, that listing them consumes a lot DynamoDB read capacity units. Because leases are revoked all the time in our setup, we run into throttled DynamoDB requests (in spikes) a lot.
To work around this, we figured that there must be an easier way to count items. Sadly this is pretty hard in DynamoDB. However what we can do is only fetch 2 items (which only consumes 1 or 2 "read capacity units" in DynamoDB). If there are no children, only one item is returned for this query - the "directory" node itself. If there are two items, we know that there are children.
We tested this in our production cluster where we previously (with v0.7.2) had the throttling issues. Everything works well and we don't see any throttled requests anymore. 🎉