Implement updated standard retry behavior#733
Conversation
| */ | ||
| public final class AwsLongPollingIntegration implements PythonIntegration { | ||
|
|
||
| private static final Map<String, Set<String>> LONG_POLLING_OPERATIONS = Map.of( |
There was a problem hiding this comment.
[Question, maybe blocking?]
Does it really make sense for us to do this now before the trait is ready? I'd rather not have to track the separate work of updating this generator to function based on the trait and just update it when the rest of the SDKs do.
If we are already committed to shipping this, it's fine, but it is extra effort on our part for no customer benefit.
There was a problem hiding this comment.
Here's what the SEP says:
IsLongPollingOperation
This step determines whether the service operation in use is a long-polling operation. Long-polling operations MUST always back off when
Retryableis true, even ifHasRetryQuotais false.An operation is considered long-polling if it includes the
aws.api#longPollSmithy trait. (longPollin C2J:"longPoll": {...}").Work to enable this trait is currently in progress, so until the trait is available, SDKs SHOULD hard-code the following combinations of service and operation as having this trait unless it requires significant wasted effort to do so.
I took a look at other smithy SDKs and almost all of them are hardcoding the table now. I also found the trait has since shipped in smithy-lang/smithy#3019, though as smithy.api#longPoll rather than aws.api#longPoll.
However, I checked the service models: none of the operations carry the trait yet.
Also, given that:
- long polling is a "SHOULD" rather than a "MUST" in SEP, and
- the SEP also allows an exception: "... unless it requires significant wasted effort to do so" (I assume we fall into this case a bit?), and
- we haven't released these clients yet,
I don't mind punting long polling to a follow-up. I agree that it is extra effort on our part for no customer benefit. If it also makes sense to you, then I'll drop the changes for long polling in this PR. Let me know what you think!
| endpoint_resolver=config.endpoint_resolver, | ||
| retry_strategy=retry_strategy, | ||
| retry_strategy=retry_strategy,${?isLongPolling} | ||
| is_long_polling=True,${/isLongPolling} |
There was a problem hiding this comment.
[blocking, change requested]
This information is retry specific. Plumbing it through every operation call works fine in this instance, but this type of solution isn't scalable. This method signature would grow very quickly if we needed to update the pipeline method's signature for every branching decision that we make as part of the pipeline call.
We should talk about the right way to plumb through the information. We already have the APIOperation object in scope. Since this will eventually be a modeled trait, doesn't it make more sense for us to add it to the operation object as opposed to growing the pipeline method's signature?
| retry_delay = self.backoff_strategy.compute_next_backoff_delay(0) | ||
| return StandardRetryToken(retry_count=0, retry_delay=retry_delay) | ||
| return StandardRetryToken( | ||
| retry_count=0, retry_delay=retry_delay, is_long_polling=is_long_polling |
There was a problem hiding this comment.
[Change requested, blocking but open to discussion]
The concept of a token represents the state of a given attempt. This concept is unrelated to whether or not the operation is used for long polling.
Plumbing this through the retry token means that refresh_retry_token_for_retry needs to copy it forward unchanged. This is more boilerplate and more complex than it needs to be.
I'm trying to think of a better alternative. Off the top of my head, we could add the operation's schema to these calls, maybe via a retries_context like concept? Or just put this long_polling in a context so that it can grow without growing the method signature?
| non-throttling errors when the customer did not configure one. Only applies | ||
| to standard mode. | ||
| """ | ||
| self._default_max_attempts = default_max_attempts |
There was a problem hiding this comment.
[Question/discussion]
Did you consider making this a service-level interceptor instead of passing it in here? The interceptor could set these values if they're unset and only apply to DDB. I think that's more inline with the intention of the library's original authors.
You may need to wait until we have provenance tracking to support this though.
|
|
||
| _LOGGER = logging.getLogger(__name__) | ||
|
|
||
| _RETRY_AFTER_HEADER = "x-amz-retry-after" |
There was a problem hiding this comment.
[Blocking]
We should not add aws-specific logic to smithy-http or other generic non-AWS specific code. We need to find a way to hook into this from the AWS specific code.
2 participants
Description of changes:
Updates the standard retry behavior:
error.is_throttling_error.RETRY_COSTis now 14, throttling retries cost 5, and the timeout-specific cost is removed.ReceiveMessage, SFNGetActivityTask, SWFPollForActivityTask/PollForDecisionTask) back off before giving up even when the quota is exhausted. Operation detection uses a per-operation codegen hook so no AWS service names leak into generic codegen.x-amz-retry-after: honored when returned by the service, capped at 5 seconds above the normal backoff; invalid/missing values fall back to normal backoff and the standard HTTPRetry-Afteris ignored.Testing:
Added unit tests for each change (throttling vs non-throttling backoff, quota costs, DynamoDB per-value defaults, the long-polling backoff matrix, and
x-amz-retry-afterparsing/clamping/wiring). Also updated affected functional tests. All Python checks pass (make check-py,make test-py) and the Java codegen builds successfully.Follow-up:
The awsJson protocol's
_create_erroris not ondevelopyet (it lives on the json-rpc branch). Once that merges, it needs the sameparse_retry_after(response)wiring that awsQuery already has, sox-amz-retry-afteris honored for awsJson services (e.g. DynamoDB) too.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.