Add standard mode retries #545
Conversation
alexgromero
force-pushed
the
standard-retries
branch
from
fc2a957 to
56af01b
Compare
jonathan343
left a comment
jonathan343
left a comment
There was a problem hiding this comment.
Thanks Alessandra!
Looks great so far. I have a few suggestions below.
| 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
There was a problem hiding this comment.
Good catch. Thanks for the suggestions! I went with the second option and the type checks are passing now.
| :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.
There was a problem hiding this comment.
That's a good point. I added it in the new revision
alexgromero
force-pushed
the
standard-retries
branch
from
56af01b to
006c5e2
Compare
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.