Implement updated standard retry behavior - #733
Conversation
e5e0c7d to
18d19e6
Compare
18d19e6 to
a900e8c
Compare
51e469d to
7b6c346
Compare
…ent pyright issue
| @@ -0,0 +1,4 @@ | |||
| { | |||
| "type": "feature", | |||
There was a problem hiding this comment.
Why does this need to be a minor bump?
There was a problem hiding this comment.
I think enhancement is more appropriate. Addressed in my latest commit. Let me know if you think otherwise!
| transport=config.transport | ||
| ) | ||
| call = $4T( | ||
| ${?isLongPolling}operation_context = $4T({"config": config}) |
There was a problem hiding this comment.
Building on Sam's comment, I still think we can improve how we pass this information into the ClientCall.
The trait should be detectable from APIOperation without needing to code generate any context. We already have precedence for operation trait detection for the idempotency token trait plus streaming traits.
We also already have a reference to an APIOperation object inside of RequestPipeline._retry through call.operation.
We can do something like this:
except RetryError as retry_error:
if call.operation.long_polling and retry_error.retry_after is not None:
await sleep(retry_error.retry_after)
raise output_context.response
I think we may need to unconditionally set retry_error.retry_after for all quota exhaustion errors to avoid passing in the operation into refresh_retry_token_for_retry. Since we are already adding retry_after to the RetryError contract, I think that's a reasonable tradeoff.
There was a problem hiding this comment.
Good call! I agree that this aligns with our existing runtime design, so I've addressed this in my latest commit. And I've removed the temporary hard-coded fallback for services that don't apply the trait, since it would introduce codegen interface changes for temporary behavior.
| else _DYNAMODB_DEFAULT_MAX_ATTEMPTS | ||
| ), | ||
| default_backoff_scale=_DYNAMODB_DEFAULT_BACKOFF_SCALE, | ||
| backoff_strategy=$5T( |
There was a problem hiding this comment.
nit: can we explicitly set max_backoff to a constant that equals 20? in case the underlying default changes, we want to make that explicit here.
| :param retry_after: An optional delay in seconds that the caller should wait before | ||
| giving up on retries. Long-polling operations use this to back off even when the | ||
| retry quota is exhausted. | ||
| :param retry_after: An optional delay in seconds that would have applied to the |
There was a problem hiding this comment.
tiny nit : "would have applied" --> "could apply"
|
|
||
| @Override | ||
| public List<RuntimeClientPlugin> getClientPlugins(GenerationContext context) { | ||
| final String pluginFile = "retry"; |
There was a problem hiding this comment.
nit: to stay consistent with our runtime libraries, can we call this retries.py.
| class LongPollTrait(Trait, id=ShapeID("smithy.api#longPoll")): | ||
| """Indicates that the service may hold the request open while waiting for | ||
| information to become available.""" | ||
|
|
There was a problem hiding this comment.
nit: Let's move the assertions to a __post_init__ method to stay consistent with other traits. See XMLNamespaceTrait for a reference. This ensures it fails at initialization not when we access the property.
Another reviewer has reviewed the changes.
3 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.smithy.api#longPolltrait at runtime throughAPIOperation.long_polling.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.Note on long-polling detection:
Some service models don't apply the
smithy.api#longPolltrait yet. We deliberately do not hard-code those operations. Doing this would require codegen interface changes for temporary behavior. The trait lookup is the long-term design regardless. Until those service models apply the trait, those operations won't get long-polling backoff.Testing:
Added unit tests for each change (throttling vs non-throttling backoff, quota costs, the quota-exhaustion backoff contract, 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.