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.
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.