Conversation
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
left a comment
alecjacobs5401
left a comment
There was a problem hiding this comment.
LGTM with just one open question
| 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.
2 participants
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.