feat: new retries implementation#3269
Conversation
yenfryherrerafeliz
force-pushed
the
feat_new_retries
branch
from
51eb997 to
bafa59d
Compare
|
I found a few some minor issues that I am addressing. One of them is that for dynamodb is still defaulting the mode to legacy due to how the configuration is created for dynamodb. Also some unused variables that I need to remove. |
yenfryherrerafeliz
force-pushed
the
feat_new_retries
branch
from
87c87bf to
75adfbd
Compare
yenfryherrerafeliz
force-pushed
the
feat_new_retries
branch
2 times, most recently
from
18a7acc to
9bd8b63
Compare
| /** @internal */ | ||
| public static function _defaultRetries(): callable | ||
| { | ||
| return \Aws\Retry\ConfigurationProvider::chain( |
There was a problem hiding this comment.
can defaultProvider() not be used here?
There was a problem hiding this comment.
I though on it, but how would you set the max attempts to four?, which is what is being handled here.
yenfryherrerafeliz
force-pushed
the
feat_new_retries
branch
2 times, most recently
from
34dee09 to
2e9958a
Compare
stobrien89
left a comment
stobrien89
left a comment
There was a problem hiding this comment.
Just one nit, but approved otherwise. Let's hold off on merging until we find out when customers will be notified about this
| $retryAfterHeader = $response?->getHeaderLine( | ||
| self::RETRY_AFTER_HEADER | ||
| ) ?? null; | ||
| } elseif($value instanceof ResultInterface) { |
There was a problem hiding this comment.
Nit: space needed after elseif
There was a problem hiding this comment.
I think the docblock was just incorrect- intval() returns an int, so it should have been returning an int previously
Adds an opt-in new retry behavior. Set AWS_NEW_RETRIES_2026=true to enable the new path. When the env var is unset (the default), retry behavior is unchanged from previous releases. With the flag enabled, the SDK switches the default retry mode from 'legacy' to 'standard', adopts a throttling-aware token-bucket retry quota (cost 14 for non-throttling, 5 for throttling), reduces the non-throttling base backoff to 50ms, checks max-attempts before quota, honors the x-amz-retry-after header, sleeps without retrying on long-polling operations (SQS, SFN, SWF) when the quota is exhausted, and lets custom deciders supplement (rather than replace) built-in retryability checks. DynamoDB defaults to 4 attempts with a 25ms base; STS treats IDPCommunicationError as transient; S3's existing custom decider keeps its socket carve-out. The flag is intended as an opt-in for early adopters and will become the default in a future release.
yenfryherrerafeliz
force-pushed
the
feat_new_retries
branch
from
003b011 to
6eccca5
Compare
stobrien89
left a comment
stobrien89
left a comment
There was a problem hiding this comment.
Looks good- clean way to handle this. A few small things and one structural recommendation: Retry\Standard implies there will only be logic handling the standard retry mode, but the new middleware also handles adaptive. I would just put all of these in the Retry namespace and rename QuotaManager to something like RetryQuota.
| public static function isEnabled(): bool | ||
| { | ||
| if (self::$enabled === null) { | ||
| self::$enabled = getenv(self::ENV) === 'true'; |
There was a problem hiding this comment.
should be a bit more permissive with the value here. See \Aws\boolean_value()
| private static function appendLegacyModeRetries( | ||
| $value, | ||
| RetryConfigurationInterface $config, | ||
| array &$args, | ||
| HandlerList $list | ||
| ): void { |
There was a problem hiding this comment.
Seeing a few places in multi-line method signatures where the opening brace needs to be moved down:
| private static function appendLegacyModeRetries( | |
| $value, | |
| RetryConfigurationInterface $config, | |
| array &$args, | |
| HandlerList $list | |
| ): void { | |
| private static function appendLegacyModeRetries( | |
| $value, | |
| RetryConfigurationInterface $config, | |
| array &$args, | |
| HandlerList $list | |
| ): void | |
| { |
| if ( | ||
| NewRetriesOptIn::isEnabled() | ||
| && is_array($value) | ||
| && !isset($value['max_attempts']) | ||
| ) { |
There was a problem hiding this comment.
I think this is the only one, but I'd check for others:
| if ( | |
| NewRetriesOptIn::isEnabled() | |
| && is_array($value) | |
| && !isset($value['max_attempts']) | |
| ) { | |
| if (NewRetriesOptIn::isEnabled() | |
| && is_array($value) | |
| && !isset($value['max_attempts']) | |
| ) { |
| use Aws\Retry\ConfigurationInterface as RetryConfigInterface; | ||
| use Aws\Retry\ConfigurationProvider as RetryConfigProvider; | ||
| use Aws\Retry\Standard\OptIn as NewRetriesOptIn; | ||
| use Aws\Retry\Standard\RetryMiddleware as StandardRetryMiddleware; |
There was a problem hiding this comment.
recommend a rename here since this middleware also handles adaptive mode
2 participants
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.