feat!: Switch from min throttle delay to retry mode #488
Conversation
This file contains hidden or 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
The v2 AWS SDK has no equivalent setting for a minimum delay for throttle errors. So, the delay has been replaced with the new retry mode configuration setting that is available. The "adaptive" mode accounts for throttle errors, although it is not configurable any further than that, and the mode itself is still experimental. The choice of retry mode still only applies to the SSM parameter store, but it could be extended to other implementations very easily. The `--min-throttle-delay` option remains in place so that existing scripted calls don't break, but the value no longer has any effect. The README is updated accordingly, mentioning the loss of min throttle delay as a breaking change. The removal of the `CHAMBER_NO_PATHS` avoiding of the path-based API for the SSM parameter store is also added as a breaking change, although that already happened when chamber moved to the v2 SDK.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
alecjacobs5401
approved these changes
May 29, 2024
| @@ -74,7 +77,12 @@ var RootCmd = &cobra.Command{ | |||
|
|
|||
| func init() { | |||
| RootCmd.PersistentFlags().IntVarP(&numRetries, "retries", "r", DefaultNumRetries, "For SSM or Secrets Manager, the number of retries we'll make before giving up; AKA $CHAMBER_RETRIES") | |||
| RootCmd.PersistentFlags().DurationVarP(&minThrottleDelay, "min-throttle-delay", "", store.DefaultMinThrottleDelay, "For SSM, minimal delay before retrying throttled requests. Default 500ms.") | |||
| RootCmd.PersistentFlags().DurationVarP(&minThrottleDelay, "min-throttle-delay", "", 0, "DEPRECATED and no longer has any effect. Use retry-mode instead") | |||
There was a problem hiding this comment.
is there any value in keeping this around if we're removing the effect that it has anyway? This will just (for the most part) silently break for people who might not deeply be reading the changelog as opposed to removing the flag or throwing an error if it is used.
There was a problem hiding this comment.
My thinking was that existing scripts that use chamber would break if the option were removed. I'm open to dropping the option if we decide that it's better to do so.
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.
The v2 AWS SDK has no equivalent setting for a minimum delay for
throttle errors. So, the delay has been replaced with the new retry mode
configuration setting that is available. The "adaptive" mode accounts
for throttle errors, although it is not configurable any further than
that, and the mode itself is still experimental.
The choice of retry mode still only applies to the SSM parameter store,
but it could be extended to other implementations very easily.
The
--min-throttle-delayoption remains in place so that existingscripted calls don't break, but the value no longer has any effect.
The README is updated accordingly, mentioning the loss of min throttle
delay as a breaking change. The removal of the
CHAMBER_NO_PATHSavoiding of the path-based API for the SSM parameter store is also added
as a breaking change, although that already happened when chamber moved
to the v2 SDK.