-
Notifications
You must be signed in to change notification settings - Fork 813
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Batch EC2 DescribeSnapshots #1958
Batch EC2 DescribeSnapshots #1958
Conversation
Code Coverage Diff
|
/hold Holding until we decide on Snapshot scalability tests. |
d2dc135
to
09fc730
Compare
09fc730
to
08b0318
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to increasing the max batch value to 1000
, otherwise lgtm!
/lgtm
08b0318
to
d842488
Compare
d842488
to
3cdea89
Compare
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: torredil The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/unhold |
Is this a bug fix or adding new feature?
Feature
What is this PR about? / Why do we need it?
Coalesces EC2 DescribeSnapshots calls across CreateSnapshot/DeleteSnapshot/ListSnapshots RPCs by Snapshot ID or by tag.
This decreases the likelihood of being exceeding one's Non-mutating API request token limit when creating many snapshots at once, which may impact all driver RPCs (ie being throttled for making too many
Describe*
calls).What testing is done?
Manual testing on modified Volume Snapshots example to check that calls were batched.
TODO manual testing on ListSnapshots RPC.
possible snapshot scalability test. We would tune snapshot batcherOffline decided to start with conservative maxDelay, and tune once snapshot scalability tests are prioritized.maxDelay
s based on these tests.CI