Enable configuring retryable status codes #3243
Conversation
chlowell
requested review from
LarryOsterman,
RickWinter and
heaths
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR adds the ability to configure which HTTP status codes should trigger retries in the retry policy. Previously, retries were attempted for a fixed set of status codes. Now, users can customize this list through a new with_retry_statuses() builder method on RetryOptions.
Key changes:
- Adds a new
get_retry_status_codes()method to theRetryPolicytrait - Implements configurable status codes in both
FixedRetryPolicyandExponentialRetryPolicy - Provides a fluent builder API through
RetryOptions::with_retry_statuses()
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/core/typespec_client_core/src/http/policies/retry/mod.rs | Adds get_retry_status_codes() to RetryPolicy trait, renames constant to DEFAULT_RETRY_STATUSES, updates retry logic, adds comprehensive tests |
| sdk/core/typespec_client_core/src/http/policies/retry/fixed.rs | Implements configurable status codes in FixedRetryPolicy with new field and getter method |
| sdk/core/typespec_client_core/src/http/policies/retry/exponential.rs | Implements configurable status codes in ExponentialRetryPolicy with new field and getter method |
| sdk/core/typespec_client_core/src/http/options/retry.rs | Adds with_retry_statuses() builder method and status_codes field to RetryOptions |
| sdk/core/typespec_client_core/CHANGELOG.md | Documents the new feature in the changelog |
| pub fn exponential(options: ExponentialRetryOptions) -> Self { | ||
| Self { | ||
| mode: RetryMode::Exponential(options), | ||
| status_codes: None, |
There was a problem hiding this comment.
..Default::default() to future proof this in case there are other RetryOptions added in the future?
There was a problem hiding this comment.
You can't if not non-exhaustive. And if in the same crate as the definition, it won't matter anyway because it forces you to always handle all cases. That only applies to callers.
| self | ||
| } | ||
|
|
||
| pub(crate) fn to_policy(&self, retry_headers: RetryHeaders) -> Arc<dyn Policy> { |
There was a problem hiding this comment.
I know it's an existing call, but should to_policy consume self rather than borrowing self? Possibly after renaming it into_policy to avoid a clippy warning?
The .clone() below just annoys me :).
|
|
||
| /// The status codes that should trigger retries. If `None`, the default set of status codes | ||
| /// will be used as described in the documentation for [`RetryOptions::with_retry_statuses`]. | ||
| status_codes: Option<Vec<StatusCode>>, |
There was a problem hiding this comment.
While leaf nodes of nested options should generally be Option<T>, that's not true of Vec and HashMap which are ZSTs. It just makes them harder. We only do this for models because we need to know the difference of empty (Vec::new().len() == 0) and absent.
Also, are we sure we want this user-settable? If client-settable, it should go in PipelineSendOptions like @LarryOsterman's recent change for CheckSuccessOptions that contains a success_codes: &'static [u16], which is more general purpose for client-defined values.
You said that Identity needs this, so it seems more like client-define options, not user-defined options.
There was a problem hiding this comment.
are we sure we want this user-settable?
No. Keeping this out of the top-level public API makes sense to me because I expect SDK developers to know which codes from a given service are retriable. Clients should be able to handle this internally and application developers shouldn't need to think about it, and we have RetryMode::Custom for an escape hatch just in case.
If client-settable, it should go in
PipelineSendOptions
I suppose I can live with this but it seems odd to have clients specify retriable status codes with every request. That design seems to imply wanting to retry different codes for different operations, and I don't know a real scenario like that.
There was a problem hiding this comment.
There are also PipelineOptions you pass when setting up the Pipeline.
| pub fn exponential(options: ExponentialRetryOptions) -> Self { | ||
| Self { | ||
| mode: RetryMode::Exponential(options), | ||
| status_codes: None, |
There was a problem hiding this comment.
You can't if not non-exhaustive. And if in the same crate as the definition, it won't matter anyway because it forces you to always handle all cases. That only applies to callers.
3 participants
I guess this feature won't often be used, however I know a couple uses of it with the Go SDK and
azure_identityneeds either this or a custom retry policy to implement correct behavior for IMDS. A builder method onRetryOptionslooks like the simplest and most ergonomic approach but I'm happy to consider alternatives.