Add standard mode retries #545
Conversation
alexgromero
force-pushed
the
standard-retries
branch
from
fc2a957 to
56af01b
Compare
| ### Features | ||
|
|
||
| * Added a hand-written implmentation for the `restJson1` protocol. | ||
| * Added a new retry mode `standard` and made it the default retry strategy. |
There was a problem hiding this comment.
Is this considered a feature or breaking change since we're updating a default?
There was a problem hiding this comment.
In boto3 it would be considered breaking, so I'm leaning towards saying "breaking change" here as well since it impacts our user's retry strategy.
The difference between making this a feature or a breaking change isn't significant enough for us to spend a ton of time discussing though, so do what feels right to you.
| async def refresh_retry_token_for_retry( | ||
| self, | ||
| *, | ||
| token_to_renew: StandardRetryToken, |
There was a problem hiding this comment.
This is going to result in the following typechecker errors:
/.../smithy-python/packages/smithy-core/src/smithy_core/retries.py
/.../smithy-python/packages/smithy-core/src/smithy_core/retries.py:288:15 - error: Method "refresh_retry_token_for_retry" overrides class "RetryStrategy" in an incompatible manner
Keyword parameter "token_to_renew" type mismatch: base parameter is type "RetryToken", override parameter is type "StandardRetryToken"
"RetryToken" is not assignable to "StandardRetryToken" (reportIncompatibleMethodOverride)
/.../smithy-python/packages/smithy-core/src/smithy_core/retries.py:331:15 - error: Method "record_success" overrides class "RetryStrategy" in an incompatible manner
Keyword parameter "token" type mismatch: base parameter is type "RetryToken", override parameter is type "StandardRetryToken"
"RetryToken" is not assignable to "StandardRetryToken" (reportIncompatibleMethodOverride)
I think we need to be using retries_interface.RetryToken here instead of StandardRetryToken.
| token_to_renew: StandardRetryToken, | |
| token_to_renew: retries_interface.RetryToken, |
Making this change alone will result in even more typing errors because StandardRetryToken has extra attributes. I think we have the following options:
- Use
typing.castto tell the typechecker we're usingStandardRetryToken - Add a runtime check to validate
StandardRetryTokenis being used
| :param max_attempts: Upper limit on total number of attempts made, including | ||
| initial attempt and retries. | ||
| """ | ||
| self.backoff_strategy = ExponentialRetryBackoffStrategy( |
There was a problem hiding this comment.
I know the max_backoff is 20 seconds and it would be a bit redundant to specify that here, but I feel like that is the safer thing to do. If for some reason we decided that a different default is better for non-AWS clients, I don't want us to accidentally change the value used here.
| initial attempt and retries. | ||
| """ | ||
| self.backoff_strategy = ExponentialRetryBackoffStrategy( | ||
| backoff_scale_value=1, |
There was a problem hiding this comment.
nit - Not really relevant to this PR, but we should consider adding some simple validation for these values on initialization (e.g., ensure they're positive integers).
There was a problem hiding this comment.
Why is the default 1? Isn't it supposed to be 2 per the specification?
Editing - I was wrong. The backoff jitter calculations are a bit complex, but in this case the final calculation winds up being random() * min(max_backoff, backoff_scale_value * (2**(attempts - 1))); when backoff_scale_value is set to 1, this is exactly what we want from the spec. The value of 2 that I was expecting is hard-coded here.
We should consider refactoring the jitter calculations to simplify them. Right now it's kind of unapproachable, which will make it harder for end-users to understand the options available and make use of them. We probably don't need to keep all four types. But for now, you can ignore my comment.
Jonathan's comment above is still relevant.
| ### Features | ||
|
|
||
| * Added a hand-written implmentation for the `restJson1` protocol. | ||
| * Added a new retry mode `standard` and made it the default retry strategy. |
There was a problem hiding this comment.
In boto3 it would be considered breaking, so I'm leaning towards saying "breaking change" here as well since it impacts our user's retry strategy.
The difference between making this a feature or a breaking change isn't significant enough for us to spend a ton of time discussing though, so do what feels right to you.
| initial attempt and retries. | ||
| """ | ||
| self.backoff_strategy = ExponentialRetryBackoffStrategy( | ||
| backoff_scale_value=1, |
There was a problem hiding this comment.
Why is the default 1? Isn't it supposed to be 2 per the specification?
Editing - I was wrong. The backoff jitter calculations are a bit complex, but in this case the final calculation winds up being random() * min(max_backoff, backoff_scale_value * (2**(attempts - 1))); when backoff_scale_value is set to 1, this is exactly what we want from the spec. The value of 2 that I was expecting is hard-coded here.
We should consider refactoring the jitter calculations to simplify them. Right now it's kind of unapproachable, which will make it harder for end-users to understand the options available and make use of them. We probably don't need to keep all four types. But for now, you can ignore my comment.
Jonathan's comment above is still relevant.
3 participants
Description of changes:
Adds support for a new retry mode:
standard. This behavior is now consistent with other AWS SDKs implementing standard mode. This mode also adds support for retry quotas which control how many unsuccessful retries a client can make.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.