From a7d5fb004992e6783bb7315e77eab90590db6cf9 Mon Sep 17 00:00:00 2001 From: Zelda Hessler Date: Mon, 2 Oct 2023 19:04:48 +0000 Subject: [PATCH 1/9] add retry classifier customization RFC add SUMMARY link to new RFC --- design/src/SUMMARY.md | 1 + .../rfc0037_retry_classifier_customization.md | 240 ++++++++++++++++++ 2 files changed, 241 insertions(+) create mode 100644 design/src/rfcs/rfc0037_retry_classifier_customization.md diff --git a/design/src/SUMMARY.md b/design/src/SUMMARY.md index a8277d72f3..9acf8d3e64 100644 --- a/design/src/SUMMARY.md +++ b/design/src/SUMMARY.md @@ -63,6 +63,7 @@ - [RFC-0034: Smithy Orchestrator](./rfcs/rfc0034_smithy_orchestrator.md) - [RFC-0035: Collection Defaults](./rfcs/rfc0035_collection_defaults.md) - [RFC-0036: HTTP Dependency Exposure](./rfcs/rfc0036_http_dep_elimination.md) + - [RFC-0037: User-configurable retry classification](./rfcs/rfc0037_retry_classifier_customization.md) - [Contributing](./contributing/overview.md) - [Writing and debugging a low-level feature that relies on HTTP](./contributing/writing_and_debugging_a_low-level_feature_that_relies_on_HTTP.md) diff --git a/design/src/rfcs/rfc0037_retry_classifier_customization.md b/design/src/rfcs/rfc0037_retry_classifier_customization.md new file mode 100644 index 0000000000..21ee9bcb0c --- /dev/null +++ b/design/src/rfcs/rfc0037_retry_classifier_customization.md @@ -0,0 +1,240 @@ +# RFC: User-configurable retry classification + +> Status: RFC +> +> Applies to: client + +For a summarized list of proposed changes, see the [Changes Checklist](#changes-checklist) section. + +This RFC defines the user experience and implementation of user-configurable retry classification. Custom retry classifiers enable users to change what errors are retried while still allowing them to rely on defaults set by SDK authors when desired. +## Terminology + +- **Smithy Service**: An HTTP service, whose API is modeled with the [Smithy IDL](https://www.smithy.io). +- **Smithy Client**: An HTTP client generated by smithy-rs from a `.smithy` model file. +- **AWS SDK**: A **smithy client** that's specifically configured to work with an AWS service. +- **Operation**: A modeled interaction with a service, defining the proper input and expected output types, as well as important metadata related to request construction. "Sending" an operation implies sending one or more HTTP requests to **smithy service**, and then receiving an output or error in response. +- **Orchestrator**: The client code which manages the request/response pipeline. The orchestrator is responsible for: + - Constructing, serializing, and sending requests. + - Receiving, deserializing, and (optionally) retrying requests. + - Running interceptors *(not covered in this RFC)* and handling errors. +- **Runtime Component**: A part of the orchestrator responsible for a specific function. Runtime components are *always* required and may depend on specific configuration. Examples include the endpoint resolver, retry strategy, and request signer. +- **Runtime Plugin**: Code responsible for setting and **runtime components** and related configuration. Runtime plugins defined by codegen are responsible for setting default configuration and altering the behavior of **smithy clients** including the **AWS SDKs**. +- **Retry Strategy**: The process by which the orchestrator determines when and how to retry failed requests. Only one retry strategy may be set. The retry strategy depends upon the **retry classifier** to interpret responses and determine if they are retryable. +- **Retry Classifier**: Code responsible for introspecting the request/response pipeline's and determining if a retry is necessary. Multiple retry classifiers may be combined into a **retry classifier chain**. +- **Retry Classifier Chain**: Requests can fail for any number of reasons, but retry classifiers are specific, meaning that they target a narrow range of possibilities. Rather than define a single retry classifier to handle all possible outcomes, multiple classifiers are chained together. Each classifier is run in turn, and the chain ends when a classifier decides a response is retryable, or a classifier may determine that a response must not be retried, or after all classifiers have run and haven't decided a retry is necessary. +- **Retry Classifier Priority**: Retry classifiers in different places and times before sending a request. Each classifier has a defined priority that enables them to be sorted correctly. When implementing your own classifier, you may set your own priority. +## The user experience if this RFC is implemented + +In the current version of the SDK, users are unable to configure retry classification, except by defining a custom retry strategy. Once this RFC is implemented, users will be able to define their own classifiers and set them at the service level and/or the operation level. + +### Defining a custom retry classifier + +```rust +use aws_smithy_runtime::client::retries::{ClassifyRetry, RetryClassifierResult}; +use aws_smithy_runtime::client::interceptors::context::InterceptorContext; + +#[derive(Debug)] +struct CustomRetryClassifier; + +impl ClassifyRetry for CustomRetryClassifier { + fn classify_retry( + &self, + ctx: &InterceptorContext, + result_of_preceding_classifier: Option, + ) -> Option { + // It's typical, but not required, to respect the judgement of the + // preceding classifier and forward it on. + if let Some(result) = result_of_preceding_classifier { + return result; + } + + todo!("inspect the interceptor context to determine if a retry attempt should be made.") + } + + fn name(&self) -> &'static str { "my custom retry classifier" } +} +``` +### Customizing retry classification for a service + +```rust +#[tokio::main] +async fn main() -> Result<(), aws_sdk_s3::Error> { + let sdk_config = aws_config::load_from_env().await; + let service_config = aws_sdk_s3::Config::from(&sdk_config) + .to_builder() + .retry_classifier(CustomRetryClassifier) + .build() + let client = aws_sdk_s3::Client::from_conf(&service_config); + + let res = client + .list_buckets() + .send() + .await?; + + println!("your buckets: {res:?}"); + + Ok(()) +} +``` + +### Customizing retry classification for an operation + +```rust +#[tokio::main] +async fn main() -> Result<(), aws_sdk_s3::Error> { + let sdk_config = aws_config::load_from_env().await; + let client = aws_sdk_s3::Client::new(&sdk_config); + + let res = client + .list_buckets() + .customize() + .await + .unwrap() + .config_override( + aws_sdk_s3::Config::builder() + .retry_classifier(CustomRetryClassifier) + ) + .send() + .await?; + + println!("your buckets: {res:?}"); + + Ok(()) +} +``` + +## How to actually implement this RFC + +In order to implement this feature, we must: +- Update the current retry classification system so that individual classifiers as well as collections of classifiers can be easily composed together. +- Create two new configuration mechanisms for users that allow them to customize retry classification at the service level and at the operation level. +- Update retry classifiers so that they may 'short-circuit' the chain, ending retry classification immediately. + +### The `RetryClassifier` trait + +```rust +/// The result of running a [`ClassifyRetry`] on a [`InterceptorContext`]. +#[non_exhaustive] +#[derive(Clone, Eq, PartialEq, Debug)] +pub enum RetryClassifierResult { + /// "A retryable error was received. This is what kind of error it was, + /// in case that's important." + Error(ErrorKind), + /// "The server told us to wait this long before retrying the request." + Explicit(Duration), + /// "This response should not be retried." + DontRetry, +} + +/// Classifies what kind of retry is needed for a given [`InterceptorContext`]. +pub trait ClassifyRetry: Send + Sync + fmt::Debug { + /// Run this classifier on the [`InterceptorContext`] to determine if the previous request + /// should be retried. Returns a [`RetryClassifierResult`]. + fn classify_retry( + &self, + ctx: &InterceptorContext, + result_of_preceding_classifier: Option, + ) -> Option; + + /// The name of this retry classifier. + /// + /// Used for debugging purposes + fn name(&self) -> &'static str; + + /// The priority of this retry classifier. Classifiers with a higher priority will run before + /// classifiers with a lower priority. Classifiers with equal priorities make no guarantees + /// about which will run first. + fn priority(&self) -> RetryClassifierPriority { + RetryClassifierPriority::default() + } +} +``` + +### Chaining retry classifiers + +Multiple retry classifiers are chained by wrapping classifiers inside one another. When classifiers must be wrapped in a specific order, use a specific type for the inner classifier. When classifiers must be composable in any order, use a referenced trait object. This approach is demonstrated in the following example code: + +```rust +struct ExampleRetryClassifier<'a> { + inner: Option<&'a dyn ClassifyRetry> +} + +impl<'a> ClassifyRetry for ExampleRetryClassifier<'a> { + fn classify_retry( + &self, + ctx: &InterceptorContext, + result_of_preceding_classifier: Option, + ) -> Option { + // It's typical, but not required, to respect the judgement of the + // preceding classifier and forward it on. + if let Some(result) = result_of_preceding_classifier { + return result; + } + + // Do retry classification here... + // Assume that we found a retryable server error + Some(RetryClassifierResult::Error(ErrorKind::ServerError)) + } +} +``` + +When each classifier in the chain reports the result of running it, debugging the result of classification is easy: + +```txt +running 'errors modeled as retryable' classifier resulted in 'continue' +running 'retryable smithy errors' classifier resulted in 'continue' +running 'http status code' classifier resulted in 'retry (server error)' +``` +### The retry classifier state machine and `RetryClassifierResult` + +It's up to each chained classifier to respect the decision made by earlier links in the chain. When properly chained, the classifiers can be thought of as a state machine: + +```mermaid +flowchart TB; + RetryStrategy --calls--> + RetryClassifier[The next retry classifier\n in the chain] --SomeError--> Retry[make a retry attempt] + RetryClassifier --SomeExplicit--> RetryAfter[wait for the given duration\nbefore making a retry attempt] + RetryClassifier --SomeDontRetry--> DontRetry[don't make a retry attempt] + RetryClassifier --None--> RetryClassifier +``` +It is possible for a wrapping classifier to ignore inner classifiers, but this is not considered typical behavior. The cases where an inner classifier would be ignored MUST be clearly documented. + +### Setting a retry classifier with a runtime plugin + +Wrapping retry classifiers may be set with a runtime plugin. When setting a classifier with this method, the runtime plugin is responsible for extracting any previously-set classifier and wrapping it. + +```rust +impl<'a> ExampleRetryClassifier<'a> { + pub fn new(inner: Option<&'a dyn ClassifyRetry>) -> Self { + Self { inner } + } +} + +struct ExampleRetryClassifierRuntimePlugin; + +impl RuntimePlugin for ExampleRetryClassifierRuntimePlugin { + fn runtime_components( + &self, + current_components: &RuntimeComponentsBuilder, + ) -> Cow<'_, RuntimeComponentsBuilder> { + + let rcb = RuntimeComponentsBuilder::new("ExampleRetryClassifierRuntimePlugin") + .with_retry_classifier( + ExampleRetryClassifier::new(current_components.retry_classifier()) + ); + + Cow::Owned(rcb) + } +} +``` + +By default, newer runtime plugins will override previously-set plugins. This is important to consider when deciding how your classifier will wrap other classifiers. + +## Changes checklist + +- [ ] Make retry classifiers composable by runtime plugins. +- [ ] Enable configuration of retry classifiers at the service level. +- [ ] Enable configuration of retry classifiers at the operation level. +- [ ] Replace `RetryReason` with `RetryClassifierResult`. + - [ ] Add variant for `DontRetry` + - [ ] Add variant for `Continue` From 3a940bb3016a78ea2d5a35578bd985740a366163 Mon Sep 17 00:00:00 2001 From: Zelda Hessler Date: Tue, 3 Oct 2023 11:43:06 -0500 Subject: [PATCH 2/9] Apply suggestions from code review Co-authored-by: John DiSanti --- .../src/rfcs/rfc0037_retry_classifier_customization.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/design/src/rfcs/rfc0037_retry_classifier_customization.md b/design/src/rfcs/rfc0037_retry_classifier_customization.md index 21ee9bcb0c..17fe5bf4ec 100644 --- a/design/src/rfcs/rfc0037_retry_classifier_customization.md +++ b/design/src/rfcs/rfc0037_retry_classifier_customization.md @@ -6,21 +6,21 @@ For a summarized list of proposed changes, see the [Changes Checklist](#changes-checklist) section. -This RFC defines the user experience and implementation of user-configurable retry classification. Custom retry classifiers enable users to change what errors are retried while still allowing them to rely on defaults set by SDK authors when desired. +This RFC defines the user experience and implementation of user-configurable retry classification. Custom retry classifiers enable users to change what responses are retried while still allowing them to rely on defaults set by SDK authors when desired. ## Terminology - **Smithy Service**: An HTTP service, whose API is modeled with the [Smithy IDL](https://www.smithy.io). - **Smithy Client**: An HTTP client generated by smithy-rs from a `.smithy` model file. - **AWS SDK**: A **smithy client** that's specifically configured to work with an AWS service. -- **Operation**: A modeled interaction with a service, defining the proper input and expected output types, as well as important metadata related to request construction. "Sending" an operation implies sending one or more HTTP requests to **smithy service**, and then receiving an output or error in response. +- **Operation**: A modeled interaction with a service, defining the proper input and expected output shapes, as well as important metadata related to request construction. "Sending" an operation implies sending one or more HTTP requests to a **Smithy service**, and then receiving an output or error in response. - **Orchestrator**: The client code which manages the request/response pipeline. The orchestrator is responsible for: - Constructing, serializing, and sending requests. - Receiving, deserializing, and (optionally) retrying requests. - Running interceptors *(not covered in this RFC)* and handling errors. -- **Runtime Component**: A part of the orchestrator responsible for a specific function. Runtime components are *always* required and may depend on specific configuration. Examples include the endpoint resolver, retry strategy, and request signer. -- **Runtime Plugin**: Code responsible for setting and **runtime components** and related configuration. Runtime plugins defined by codegen are responsible for setting default configuration and altering the behavior of **smithy clients** including the **AWS SDKs**. +- **Runtime Component**: A part of the orchestrator responsible for a specific function. Runtime components are used by the orchestrator itself, may depend on specific configuration, and must not be changed by interceptors. Examples include the endpoint resolver, retry strategy, and request signer. +- **Runtime Plugin**: Code responsible for setting and **runtime components** and related configuration. Runtime plugins defined by codegen are responsible for setting default configuration and altering the behavior of **Smithy clients** including the **AWS SDKs**. - **Retry Strategy**: The process by which the orchestrator determines when and how to retry failed requests. Only one retry strategy may be set. The retry strategy depends upon the **retry classifier** to interpret responses and determine if they are retryable. -- **Retry Classifier**: Code responsible for introspecting the request/response pipeline's and determining if a retry is necessary. Multiple retry classifiers may be combined into a **retry classifier chain**. +- **Retry Classifier**: Code responsible for introspecting the request/response pipelines and determining if a retry is necessary. Multiple retry classifiers may be combined into a **retry classifier chain**. - **Retry Classifier Chain**: Requests can fail for any number of reasons, but retry classifiers are specific, meaning that they target a narrow range of possibilities. Rather than define a single retry classifier to handle all possible outcomes, multiple classifiers are chained together. Each classifier is run in turn, and the chain ends when a classifier decides a response is retryable, or a classifier may determine that a response must not be retried, or after all classifiers have run and haven't decided a retry is necessary. - **Retry Classifier Priority**: Retry classifiers in different places and times before sending a request. Each classifier has a defined priority that enables them to be sorted correctly. When implementing your own classifier, you may set your own priority. ## The user experience if this RFC is implemented From 088209e279a9de23295abbbbac1de36788c4890c Mon Sep 17 00:00:00 2001 From: Zelda Hessler Date: Fri, 6 Oct 2023 09:39:53 -0500 Subject: [PATCH 3/9] Update design/src/rfcs/rfc0037_retry_classifier_customization.md Co-authored-by: John DiSanti --- design/src/rfcs/rfc0037_retry_classifier_customization.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/design/src/rfcs/rfc0037_retry_classifier_customization.md b/design/src/rfcs/rfc0037_retry_classifier_customization.md index 17fe5bf4ec..d34456d00c 100644 --- a/design/src/rfcs/rfc0037_retry_classifier_customization.md +++ b/design/src/rfcs/rfc0037_retry_classifier_customization.md @@ -116,14 +116,14 @@ In order to implement this feature, we must: /// The result of running a [`ClassifyRetry`] on a [`InterceptorContext`]. #[non_exhaustive] #[derive(Clone, Eq, PartialEq, Debug)] -pub enum RetryClassifierResult { +pub enum RetryAction { /// "A retryable error was received. This is what kind of error it was, /// in case that's important." - Error(ErrorKind), + Retry(ErrorKind), /// "The server told us to wait this long before retrying the request." - Explicit(Duration), + RetryAfter(Duration), /// "This response should not be retried." - DontRetry, + NoRetry, } /// Classifies what kind of retry is needed for a given [`InterceptorContext`]. From 728a61a456f153288b9f609a7ba87e84af4d8f60 Mon Sep 17 00:00:00 2001 From: Zelda Hessler Date: Fri, 6 Oct 2023 12:16:41 -0500 Subject: [PATCH 4/9] update RFC per comments and implementation changes --- .../rfc0037_retry_classifier_customization.md | 412 +++++++++++------- 1 file changed, 251 insertions(+), 161 deletions(-) diff --git a/design/src/rfcs/rfc0037_retry_classifier_customization.md b/design/src/rfcs/rfc0037_retry_classifier_customization.md index d34456d00c..12fbfe0be7 100644 --- a/design/src/rfcs/rfc0037_retry_classifier_customization.md +++ b/design/src/rfcs/rfc0037_retry_classifier_customization.md @@ -6,33 +6,89 @@ For a summarized list of proposed changes, see the [Changes Checklist](#changes-checklist) section. -This RFC defines the user experience and implementation of user-configurable retry classification. Custom retry classifiers enable users to change what responses are retried while still allowing them to rely on defaults set by SDK authors when desired. +This RFC defines the user experience and implementation of user-configurable +retry classification. Custom retry classifiers enable users to change what +responses are retried while still allowing them to rely on defaults set by SDK +authors when desired. ## Terminology -- **Smithy Service**: An HTTP service, whose API is modeled with the [Smithy IDL](https://www.smithy.io). -- **Smithy Client**: An HTTP client generated by smithy-rs from a `.smithy` model file. -- **AWS SDK**: A **smithy client** that's specifically configured to work with an AWS service. -- **Operation**: A modeled interaction with a service, defining the proper input and expected output shapes, as well as important metadata related to request construction. "Sending" an operation implies sending one or more HTTP requests to a **Smithy service**, and then receiving an output or error in response. -- **Orchestrator**: The client code which manages the request/response pipeline. The orchestrator is responsible for: - - Constructing, serializing, and sending requests. - - Receiving, deserializing, and (optionally) retrying requests. - - Running interceptors *(not covered in this RFC)* and handling errors. -- **Runtime Component**: A part of the orchestrator responsible for a specific function. Runtime components are used by the orchestrator itself, may depend on specific configuration, and must not be changed by interceptors. Examples include the endpoint resolver, retry strategy, and request signer. -- **Runtime Plugin**: Code responsible for setting and **runtime components** and related configuration. Runtime plugins defined by codegen are responsible for setting default configuration and altering the behavior of **Smithy clients** including the **AWS SDKs**. -- **Retry Strategy**: The process by which the orchestrator determines when and how to retry failed requests. Only one retry strategy may be set. The retry strategy depends upon the **retry classifier** to interpret responses and determine if they are retryable. -- **Retry Classifier**: Code responsible for introspecting the request/response pipelines and determining if a retry is necessary. Multiple retry classifiers may be combined into a **retry classifier chain**. -- **Retry Classifier Chain**: Requests can fail for any number of reasons, but retry classifiers are specific, meaning that they target a narrow range of possibilities. Rather than define a single retry classifier to handle all possible outcomes, multiple classifiers are chained together. Each classifier is run in turn, and the chain ends when a classifier decides a response is retryable, or a classifier may determine that a response must not be retried, or after all classifiers have run and haven't decided a retry is necessary. -- **Retry Classifier Priority**: Retry classifiers in different places and times before sending a request. Each classifier has a defined priority that enables them to be sorted correctly. When implementing your own classifier, you may set your own priority. +- **Smithy Service**: An HTTP service, whose API is modeled with the [Smithy + IDL](https://www.smithy.io). +- **Smithy Client**: An HTTP client generated by smithy-rs from a `.smithy` + model file. +- **AWS SDK**: A **smithy client** that's specifically configured to work with + an AWS service. +- **Operation**: A modeled interaction with a service, defining the proper input + and expected output shapes, as well as important metadata related to request + construction. "Sending" an operation implies sending one or more HTTP requests + to a **Smithy service**, and then receiving an output or error in response. +- **Orchestrator**: The client code which manages the request/response pipeline. + The orchestrator is responsible for: + - Constructing, serializing, and sending requests. + - Receiving, deserializing, and (optionally) retrying requests. + - Running interceptors *(not covered in this RFC)* and handling errors. +- **Runtime Component**: A part of the orchestrator responsible for a specific + function. Runtime components are used by the orchestrator itself, may depend + on specific configuration, and must not be changed by interceptors. Examples + include the endpoint resolver, retry strategy, and request signer. +- **Runtime Plugin**: Code responsible for setting and **runtime components** + and related configuration. Runtime plugins defined by codegen are responsible + for setting default configuration and altering the behavior of **Smithy + clients** including the **AWS SDKs**. + +## How the orchestrator should model retries + +A **Retry Strategy** is the process by which the orchestrator determines when +and how to retry failed requests. Only one retry strategy may be set at any +given time. During its operation, the retry strategy relies on a series of +**Retry Classifiers** to determine if and how a failed request should be +retried. Retry classifiers each have a **Retry Classifier Priority** so that +regardless of whether they are set during config or operation construction, +they'll always run in a consistent order. + +Classifiers are each run in turn by the retry strategy: + +```rust +pub fn run_classifiers_on_ctx( + classifiers: impl Iterator, + ctx: &InterceptorContext, +) -> RetryAction { + let mut result = None; + + for classifier in classifiers { + let new_result = classifier.classify_retry(ctx, result.clone()); + + // Emit a log whenever a new result overrides the result of a higher-priority classifier. + if new_result != result && new_result.is_none() { + tracing::debug!( + "Classifier '{}' has overridden the result of a higher-priority classifier", + classifier.name() + ); + } + + result = new_result; + } + + result.unwrap_or(RetryAction::NoRetry) +} +``` + +*NOTE: User-defined retry strategies are responsible for calling `run_classifiers_on_ctx`.* + +Lower-priority classifiers have the option of overriding or passing on the +`RetryAction` returned by higher-priority classifiers. However, the default +classifiers set for generic and AWS smithy clients always respect the result of +higher-priority classifiers. + ## The user experience if this RFC is implemented -In the current version of the SDK, users are unable to configure retry classification, except by defining a custom retry strategy. Once this RFC is implemented, users will be able to define their own classifiers and set them at the service level and/or the operation level. +In the current version of the SDK, users are unable to configure retry +classification, except by defining a custom retry strategy. Once this RFC is +implemented, users will be able to define and set their own classifiers. -### Defining a custom retry classifier +### Defining a custom classifier ```rust -use aws_smithy_runtime::client::retries::{ClassifyRetry, RetryClassifierResult}; -use aws_smithy_runtime::client::interceptors::context::InterceptorContext; - #[derive(Debug)] struct CustomRetryClassifier; @@ -40,69 +96,153 @@ impl ClassifyRetry for CustomRetryClassifier { fn classify_retry( &self, ctx: &InterceptorContext, - result_of_preceding_classifier: Option, - ) -> Option { - // It's typical, but not required, to respect the judgement of the - // preceding classifier and forward it on. - if let Some(result) = result_of_preceding_classifier { - return result; - } - - todo!("inspect the interceptor context to determine if a retry attempt should be made.") - } - - fn name(&self) -> &'static str { "my custom retry classifier" } + preceding_action: Option, + ) -> Option { + // It's typical, but not required, to respect the judgement of the + // preceding classifier and forward it on. + if let Some(action) = preceding_action { + return action; + } + + todo!("inspect the interceptor context to determine if a retry attempt should be made.") + } + + fn name(&self) -> &'static str { "my custom retry classifier" } + + fn priority(&self) -> RetryClassifierPriority { + RetryClassifierPriority::default() + } } ``` -### Customizing retry classification for a service + +#### Choosing a retry classifier priority + +Sticking with the default priority is often the best choice. Classifiers should +restrict the number of cases they can handle in order to avoid having to compete +with other classifiers. When two classifiers would classify a response in two +different ways, the priority system gives us the ability to decide which +classifier should be respected. + +Internally, priority is implemented with a simple numeric system. In order to +give the smithy-rs team the flexibility to make future changes, this numeric +system is private and inaccessible to users. Instead, users may set the priority +of classifiers relative to one another with the `run_after` and `run_before` +methods: ```rust -#[tokio::main] -async fn main() -> Result<(), aws_sdk_s3::Error> { - let sdk_config = aws_config::load_from_env().await; - let service_config = aws_sdk_s3::Config::from(&sdk_config) - .to_builder() - .retry_classifier(CustomRetryClassifier) - .build() - let client = aws_sdk_s3::Client::from_conf(&service_config); - - let res = client - .list_buckets() - .send() - .await?; - - println!("your buckets: {res:?}"); - - Ok(()) +impl RetryClassifierPriority { + /// Create a new `RetryClassifierPriority` that runs after the given priority. + pub fn run_after(other: Self) -> Self { + Self::Other(other.as_i8() - 1) + } + + /// Create a new `RetryClassifierPriority` that runs before the given priority. + pub fn run_before(other: Self) -> Self { + Self::Other(other.as_i8() + 1) + } } ``` -### Customizing retry classification for an operation +For example, if it was important for our `CustomRetryClassifer` in the previous +example to run *before* the default `HttpStatusCodeClassifier`, a user would +define the `CustomRetryClassifer` priority like this: ```rust -#[tokio::main] -async fn main() -> Result<(), aws_sdk_s3::Error> { - let sdk_config = aws_config::load_from_env().await; - let client = aws_sdk_s3::Client::new(&sdk_config); - - let res = client - .list_buckets() - .customize() - .await - .unwrap() - .config_override( - aws_sdk_s3::Config::builder() - .retry_classifier(CustomRetryClassifier) - ) - .send() - .await?; - - println!("your buckets: {res:?}"); - - Ok(()) +impl ClassifyRetry for CustomRetryClassifier { + fn priority(&self) -> RetryClassifierPriority { + RetryClassifierPriority::run_before(RetryClassifierPriority::http_status_code_classifier()) + } } ``` +The priorities of the three default retry classifiers +(`HttpStatusCodeClassifier`, `ModeledAsRetryableClassifier`, and +`TransientErrorClassifier`) are all public for this purpose. Users may **ONLY** +set the priority of a retry relative to an existing retry priority. + +### Setting classifiers + +The interface for setting classifiers is very similar to the interface of +settings interceptors: + +```rust +// All service configs support these setters. Operations support a nearly identical API. +impl ServiceConfigBuilder { + /// Add type implementing ClassifyRetry that will be used by the RetryStrategy + /// to determine what responses should be retried. + /// + /// A retry classifier configured by this method will run according to its priority. + pub fn retry_classifier(mut self, retry_classifier: impl ClassifyRetry + 'static) -> Self { + self.push_retry_classifier(SharedRetryClassifier::new(retry_classifier)); + self + } + + /// Add a SharedRetryClassifier that will be used by the RetryStrategy to + /// determine what responses should be retried. + /// + /// A retry classifier configured by this method will run according to its priority. + pub fn push_retry_classifier(&mut self, retry_classifier: SharedRetryClassifier) -> &mut Self { + self.runtime_components.push_retry_classifier(retry_classifier); + self + } + + /// Set SharedRetryClassifiers for the builder, replacing any that were + /// previously set. + pub fn set_retry_classifiers(&mut self, retry_classifiers: impl IntoIterator) -> &mut Self { + self.runtime_components.set_retry_classifiers(retry_classifiers.into_iter()); + self + } +} +``` + +### Default classifiers + +Smithy clients have three classifiers enabled by default: + +- `ModeledAsRetryableClassifier`: Checks for errors that are marked as retryable + in the smithy model. If one is encountered, returns + `Some(RetryAction::Retry(ErrorKind))`. Otherwise, returns `None`. Requires a + parsed response. +- `TransientErrorClassifier`: Checks for timeout, IO, and connector errors. If + one is encountered, returns + `Some(RetryAction::Retry(ErrorKind::TransientError))`. Otherwise, returns + `None`. Requires a parsed response. +- `HttpStatusCodeClassifier`: Checks the HTTP response's status code. By default + this classifies `500`, `502`, `503`, and `504` errors as + `Some(RetryAction::Retry(ErrorKind::TransientError))`. Otherwise, returns + `None`. The list of retryable status codes may be customized when creating + this classifier with the `HttpStatusCodeClassifier::new_from_codes` method. + +AWS clients enable the three smithy classifiers as well as two more by default: + +- `AwsErrorCodeClassifier`: Checks for errors with AWS error codes marking them + as either transient or throttling errors. If one is encountered, returns + `Some(RetryAction::Retry(ErrorKind))`. Otherwise, returns `None`. Requires a + parsed response. +- `AmzRetryAfterHeaderClassifier`: Checks the HTTP response for an + `x-amz-retry-after` header. If one is set, returns + `Some(RetryAction::RetryAfter(duration))` where duration is the header's + value. Otherwise, returns `None`. + +The priority order of these classifiers is as follows: + +1. *(highest priority)* `TransientErrorClassifier` +2. `AmzRetryAfterHeaderClassifier` +3. `ModeledAsRetryableClassifier` +4. `AwsErrorCodeClassifier` +5. *(lowest priority)* `HttpStatusCodeClassifier` + +The priority order of the default classifiers is not configurable. However, it's possible to wrap a default classifier in a newtype and set your desired priority when implementing the `ClassifyRetry` trait, delegating the `classify_retry` and `name` fields to the inner classifier. + +#### Disable default classifiers + +Disabling the default classifiers is possible, but not easy. They are set at different points during config and operation construction, and must be unset at each of those places. A far simpler solution is to implement your own classifier that either: + + - Has the highest priority. All default classifiers will always respect the action of a higher-priority classifier. + - Has the lowest priority and disregards the action of all other classifiers. + +Still, if completely removing the other classifiers is desired, use the `set_retry_classifiers` method on the config to replace the config-level defaults and then set a config override on the operation that does the same. + ## How to actually implement this RFC In order to implement this feature, we must: @@ -117,28 +257,29 @@ In order to implement this feature, we must: #[non_exhaustive] #[derive(Clone, Eq, PartialEq, Debug)] pub enum RetryAction { - /// "A retryable error was received. This is what kind of error it was, - /// in case that's important." + /// When an error is received that should be retried, this action is returned. Retry(ErrorKind), - /// "The server told us to wait this long before retrying the request." + /// When the server tells us to retry after a specific time has elapsed, this action is returned. RetryAfter(Duration), - /// "This response should not be retried." + /// When a response should not be retried, this action is returned. NoRetry, } -/// Classifies what kind of retry is needed for a given [`InterceptorContext`]. +/// Classifies what kind of retry is needed for a given [`InterceptorContext`]. pub trait ClassifyRetry: Send + Sync + fmt::Debug { /// Run this classifier on the [`InterceptorContext`] to determine if the previous request - /// should be retried. Returns a [`RetryClassifierResult`]. + /// should be retried. If the classifier makes a decision, `Some(RetryAction)` is returned. + /// Classifiers may also return `None`, signifying that they have no opinion of whether or + /// not a request should be retried. fn classify_retry( &self, ctx: &InterceptorContext, - result_of_preceding_classifier: Option, - ) -> Option; + preceding_action: Option, + ) -> Option; /// The name of this retry classifier. /// - /// Used for debugging purposes + /// Used for debugging purposes. fn name(&self) -> &'static str; /// The priority of this retry classifier. Classifiers with a higher priority will run before @@ -150,91 +291,40 @@ pub trait ClassifyRetry: Send + Sync + fmt::Debug { } ``` -### Chaining retry classifiers - -Multiple retry classifiers are chained by wrapping classifiers inside one another. When classifiers must be wrapped in a specific order, use a specific type for the inner classifier. When classifiers must be composable in any order, use a referenced trait object. This approach is demonstrated in the following example code: - -```rust -struct ExampleRetryClassifier<'a> { - inner: Option<&'a dyn ClassifyRetry> -} - -impl<'a> ClassifyRetry for ExampleRetryClassifier<'a> { - fn classify_retry( - &self, - ctx: &InterceptorContext, - result_of_preceding_classifier: Option, - ) -> Option { - // It's typical, but not required, to respect the judgement of the - // preceding classifier and forward it on. - if let Some(result) = result_of_preceding_classifier { - return result; - } - - // Do retry classification here... - // Assume that we found a retryable server error - Some(RetryClassifierResult::Error(ErrorKind::ServerError)) - } -} -``` - -When each classifier in the chain reports the result of running it, debugging the result of classification is easy: - -```txt -running 'errors modeled as retryable' classifier resulted in 'continue' -running 'retryable smithy errors' classifier resulted in 'continue' -running 'http status code' classifier resulted in 'retry (server error)' -``` -### The retry classifier state machine and `RetryClassifierResult` - -It's up to each chained classifier to respect the decision made by earlier links in the chain. When properly chained, the classifiers can be thought of as a state machine: - -```mermaid -flowchart TB; - RetryStrategy --calls--> - RetryClassifier[The next retry classifier\n in the chain] --SomeError--> Retry[make a retry attempt] - RetryClassifier --SomeExplicit--> RetryAfter[wait for the given duration\nbefore making a retry attempt] - RetryClassifier --SomeDontRetry--> DontRetry[don't make a retry attempt] - RetryClassifier --None--> RetryClassifier -``` -It is possible for a wrapping classifier to ignore inner classifiers, but this is not considered typical behavior. The cases where an inner classifier would be ignored MUST be clearly documented. +### Resolving the correct order of multiple retry classifiers -### Setting a retry classifier with a runtime plugin +Because each classifier has a defined priority, and because +`RetryClassifierPriority` implements `PartialOrd` and `Ord`, the standard +library's [sort] method may be used to correctly arrange classifiers. The +`RuntimeComponents` struct is responsible for storing classifiers, so it's also +responsible for sorting them whenever a new classifier is added. Thus, when a +retry strategy fetches the list of classifiers, they'll already be in the +expected order. -Wrapping retry classifiers may be set with a runtime plugin. When setting a classifier with this method, the runtime plugin is responsible for extracting any previously-set classifier and wrapping it. +## Questions and answers -```rust -impl<'a> ExampleRetryClassifier<'a> { - pub fn new(inner: Option<&'a dyn ClassifyRetry>) -> Self { - Self { inner } - } -} +- **Q:** Why are retry classifiers responsible for passing on the result of preceding +classifiers? Isn't this a 'footgun'? + - **A:** Allowing lower-priority classifiers to second-guess the action returned +by a higher-priority classifier allows users to define "catch-all" classifiers +that run last and have ultimate authority to decide if a response should be +retried. +- **Q:** Should retry classifiers be fallible? + - **A:** I think no, because of the added complexity. If we make them fallible then we'll have to decide what happens when classifiers fail. Do we skip them or does classification end? The retry strategy is responsible for calling the classifiers so it be responsible for deciding how to handle a classifier error. I don't foresee a use case where an error returned by a classifier would be interpreted either by classifiers following the failed classifier or the retry strategy. -struct ExampleRetryClassifierRuntimePlugin; - -impl RuntimePlugin for ExampleRetryClassifierRuntimePlugin { - fn runtime_components( - &self, - current_components: &RuntimeComponentsBuilder, - ) -> Cow<'_, RuntimeComponentsBuilder> { - - let rcb = RuntimeComponentsBuilder::new("ExampleRetryClassifierRuntimePlugin") - .with_retry_classifier( - ExampleRetryClassifier::new(current_components.retry_classifier()) - ); - - Cow::Owned(rcb) - } -} -``` +## Changes checklist -By default, newer runtime plugins will override previously-set plugins. This is important to consider when deciding how your classifier will wrap other classifiers. +- [ ] Add retry classifiers field and setters to `RuntimeComponents` and `RuntimeComponentsBuilder`. + - [ ] Add unit tests ensuring that classifier priority is respected by `RuntimeComponents::retry_classifiers`, especially when multiple layers of config are in play. +- [ ] Add codegen customization allowing users to set retry classifiers on service configs. +- [ ] Add codegen for setting default classifiers at the service level. + - [ ] Add integration tests for setting classifiers at the service level. +- [ ] Add codegen for settings default classifiers that require knowledge of operation error types at the operation level. + - [ ] Add integration tests for setting classifiers at the operation level. +- [ ] Implement retry classifier priority. + - [ ] Add unit tests for retry classifier priority. +- [ ] Update existing tests that would fail for lack of a retry classifier. -## Changes checklist + -- [ ] Make retry classifiers composable by runtime plugins. -- [ ] Enable configuration of retry classifiers at the service level. -- [ ] Enable configuration of retry classifiers at the operation level. -- [ ] Replace `RetryReason` with `RetryClassifierResult`. - - [ ] Add variant for `DontRetry` - - [ ] Add variant for `Continue` +[sort]: https://doc.rust-lang.org/stable/std/primitive.slice.html#method.sort From b6bd1cd5b81a384a508aa4e3085ea08ed023eaa7 Mon Sep 17 00:00:00 2001 From: Zelda Hessler Date: Tue, 10 Oct 2023 11:45:16 -0500 Subject: [PATCH 5/9] update RFC to reflect current API --- .../rfc0037_retry_classifier_customization.md | 203 +++++++++++++----- 1 file changed, 154 insertions(+), 49 deletions(-) diff --git a/design/src/rfcs/rfc0037_retry_classifier_customization.md b/design/src/rfcs/rfc0037_retry_classifier_customization.md index 12fbfe0be7..1e451d0060 100644 --- a/design/src/rfcs/rfc0037_retry_classifier_customization.md +++ b/design/src/rfcs/rfc0037_retry_classifier_customization.md @@ -53,32 +53,42 @@ pub fn run_classifiers_on_ctx( classifiers: impl Iterator, ctx: &InterceptorContext, ) -> RetryAction { - let mut result = None; + // By default, don't retry + let mut result = RetryAction::NoActionIndicated; for classifier in classifiers { - let new_result = classifier.classify_retry(ctx, result.clone()); - - // Emit a log whenever a new result overrides the result of a higher-priority classifier. - if new_result != result && new_result.is_none() { - tracing::debug!( - "Classifier '{}' has overridden the result of a higher-priority classifier", - classifier.name() - ); + let new_result = classifier.classify_retry(ctx); + + // If the result is `NoActionIndicated`, continue to the next classifier + // without overriding any previously-set result. + if new_result == RetryAction::NoActionIndicated { + continue; } + // Otherwise, set the result to the new result. + tracing::trace!( + "Classifier '{}' set the result of classification to '{}'", + classifier.name(), + new_result + ); result = new_result; + + // If the result is `RetryForbidden`, stop running classifiers. + if result == RetryAction::RetryForbidden { + tracing::trace!("retry classification ending early because a `RetryAction::RetryForbidden` was emitted",); + break; + } } - result.unwrap_or(RetryAction::NoRetry) + result } ``` *NOTE: User-defined retry strategies are responsible for calling `run_classifiers_on_ctx`.* -Lower-priority classifiers have the option of overriding or passing on the -`RetryAction` returned by higher-priority classifiers. However, the default -classifiers set for generic and AWS smithy clients always respect the result of -higher-priority classifiers. +Lower-priority classifiers run first, but the retry actions they return may be +overridden by higher-priority classifiers. Classification stops immediately if +any classifier returns `RetryAction::RetryForbidden`. ## The user experience if this RFC is implemented @@ -96,15 +106,18 @@ impl ClassifyRetry for CustomRetryClassifier { fn classify_retry( &self, ctx: &InterceptorContext, - preceding_action: Option, ) -> Option { - // It's typical, but not required, to respect the judgement of the - // preceding classifier and forward it on. - if let Some(action) = preceding_action { - return action; - } - - todo!("inspect the interceptor context to determine if a retry attempt should be made.") + // Check for a result + let output_or_error = ctx.output_or_error(); + // Check for an error + let error = match output_or_error { + // Typically, when the response is OK or unset + // then `RetryAction::NoActionIndicated` is returned. + Some(Ok(_)) | None => return RetryAction::NoActionIndicated, + Some(Err(err)) => err, + }; + + todo!("inspect the error to determine if a retry attempt should be made.") } fn name(&self) -> &'static str { "my custom retry classifier" } @@ -126,20 +139,16 @@ classifier should be respected. Internally, priority is implemented with a simple numeric system. In order to give the smithy-rs team the flexibility to make future changes, this numeric system is private and inaccessible to users. Instead, users may set the priority -of classifiers relative to one another with the `run_after` and `run_before` -methods: +of classifiers relative to one another with the `with_lower_priority_than` and +`with_higher_priority_than` methods: ```rust impl RetryClassifierPriority { - /// Create a new `RetryClassifierPriority` that runs after the given priority. - pub fn run_after(other: Self) -> Self { - Self::Other(other.as_i8() - 1) - } + /// Create a new `RetryClassifierPriority` with lower priority than the given priority. + pub fn with_lower_priority_than(other: Self) -> Self { ... } - /// Create a new `RetryClassifierPriority` that runs before the given priority. - pub fn run_before(other: Self) -> Self { - Self::Other(other.as_i8() + 1) - } + /// Create a new `RetryClassifierPriority` with higher priority than the given priority. + pub fn with_higher_priority_than(other: Self) -> Self { ... } } ``` @@ -158,7 +167,94 @@ impl ClassifyRetry for CustomRetryClassifier { The priorities of the three default retry classifiers (`HttpStatusCodeClassifier`, `ModeledAsRetryableClassifier`, and `TransientErrorClassifier`) are all public for this purpose. Users may **ONLY** -set the priority of a retry relative to an existing retry priority. +set a retry priority relative to an existing retry priority. + + +#### `RetryAction` and `RetryReason` + +Retry classifiers communicate to the retry strategy by emitting `RetryAction`s: + +```rust +/// The result of running a [`ClassifyRetry`] on a [`InterceptorContext`]. +#[non_exhaustive] +#[derive(Clone, Eq, PartialEq, Debug, Default)] +pub enum RetryAction { + /// When a classifier can't run or has no opinion, this action is returned. + /// + /// For example, if a classifier requires a parsed response and response parsing failed, + /// this action is returned. If all classifiers return this action, no retry should be + /// attempted. + #[default] + NoActionIndicated, + /// When a classifier runs and thinks a response should be retried, this action is returned. + RetryIndicated(RetryReason), + /// When a classifier runs and decides a response must not be retried, this action is returned. + /// + /// This action stops retry classification immediately, skipping any following classifiers. + RetryForbidden, +} +``` + +When a retry is indicated by a classifier, the action will contain a `RetryReason`: + +```rust +/// The reason for a retry. +#[non_exhaustive] +#[derive(Clone, Eq, PartialEq, Debug)] +pub enum RetryReason { + /// When an error is received that should be retried, this reason is returned. + RetryableError { + /// The kind of error. + kind: ErrorKind, + /// A server may tell us to retry only after a specific time has elapsed. + retry_after: Option, + }, +} +``` + +*NOTE: `RetryReason` currently only has a single variant but it's defined as an `enum` for [forward compatibility] purposes.* + +`RetryAction`'s `impl` defines several convenience methods: + +```rust +impl RetryAction { + /// Create a new `RetryAction` indicating that a retry is necessary. + pub fn retryable_error(kind: ErrorKind) -> Self { + Self::RetryIndicated(RetryReason::RetryableError { + kind, + retry_after: None, + }) + } + + /// Create a new `RetryAction` indicating that a retry is necessary after an explicit delay. + pub fn retryable_error_with_explicit_delay(kind: ErrorKind, retry_after: Duration) -> Self { + Self::RetryIndicated(RetryReason::RetryableError { + kind, + retry_after: Some(retry_after), + }) + } + + /// Create a new `RetryAction` indicating that a retry is necessary because of a transient error. + pub fn transient_error() -> Self { + Self::retryable_error(ErrorKind::TransientError) + } + + /// Create a new `RetryAction` indicating that a retry is necessary because of a throttling error. + pub fn throttling_error() -> Self { + Self::retryable_error(ErrorKind::ThrottlingError) + } + + /// Create a new `RetryAction` indicating that a retry is necessary because of a server error. + pub fn server_error() -> Self { + Self::retryable_error(ErrorKind::ServerError) + } + + /// Create a new `RetryAction` indicating that a retry is necessary because of a client error. + pub fn client_error() -> Self { + Self::retryable_error(ErrorKind::ClientError) + } +} +``` ### Setting classifiers @@ -232,23 +328,31 @@ The priority order of these classifiers is as follows: 4. `AwsErrorCodeClassifier` 5. *(lowest priority)* `HttpStatusCodeClassifier` -The priority order of the default classifiers is not configurable. However, it's possible to wrap a default classifier in a newtype and set your desired priority when implementing the `ClassifyRetry` trait, delegating the `classify_retry` and `name` fields to the inner classifier. +The priority order of the default classifiers is not configurable. However, it's +possible to wrap a default classifier in a newtype and set your desired priority +when implementing the `ClassifyRetry` trait, delegating the `classify_retry` and +`name` fields to the inner classifier. #### Disable default classifiers -Disabling the default classifiers is possible, but not easy. They are set at different points during config and operation construction, and must be unset at each of those places. A far simpler solution is to implement your own classifier that either: - - - Has the highest priority. All default classifiers will always respect the action of a higher-priority classifier. - - Has the lowest priority and disregards the action of all other classifiers. +Disabling the default classifiers is possible, but not easy. They are set at +different points during config and operation construction, and must be unset at +each of those places. A far simpler solution is to implement your own classifier +that has the highest priority. -Still, if completely removing the other classifiers is desired, use the `set_retry_classifiers` method on the config to replace the config-level defaults and then set a config override on the operation that does the same. +Still, if completely removing the other classifiers is desired, use the +`set_retry_classifiers` method on the config to replace the config-level +defaults and then set a config override on the operation that does the same. ## How to actually implement this RFC In order to implement this feature, we must: -- Update the current retry classification system so that individual classifiers as well as collections of classifiers can be easily composed together. -- Create two new configuration mechanisms for users that allow them to customize retry classification at the service level and at the operation level. -- Update retry classifiers so that they may 'short-circuit' the chain, ending retry classification immediately. +- Update the current retry classification system so that individual classifiers + as well as collections of classifiers can be easily composed together. +- Create two new configuration mechanisms for users that allow them to customize + retry classification at the service level and at the operation level. +- Update retry classifiers so that they may 'short-circuit' the chain, ending + retry classification immediately. ### The `RetryClassifier` trait @@ -303,14 +407,14 @@ expected order. ## Questions and answers -- **Q:** Why are retry classifiers responsible for passing on the result of preceding -classifiers? Isn't this a 'footgun'? - - **A:** Allowing lower-priority classifiers to second-guess the action returned -by a higher-priority classifier allows users to define "catch-all" classifiers -that run last and have ultimate authority to decide if a response should be -retried. - **Q:** Should retry classifiers be fallible? - - **A:** I think no, because of the added complexity. If we make them fallible then we'll have to decide what happens when classifiers fail. Do we skip them or does classification end? The retry strategy is responsible for calling the classifiers so it be responsible for deciding how to handle a classifier error. I don't foresee a use case where an error returned by a classifier would be interpreted either by classifiers following the failed classifier or the retry strategy. + - **A:** I think no, because of the added complexity. If we make them fallible + then we'll have to decide what happens when classifiers fail. Do we skip + them or does classification end? The retry strategy is responsible for + calling the classifiers so it be responsible for deciding how to handle a + classifier error. I don't foresee a use case where an error returned by a + classifier would be interpreted either by classifiers following the failed + classifier or the retry strategy. ## Changes checklist @@ -328,3 +432,4 @@ retried. [sort]: https://doc.rust-lang.org/stable/std/primitive.slice.html#method.sort +[forward compatibility]: https://en.wikipedia.org/wiki/Forward_compatibility From 7c6a530d35cdd7877c837e89c10b22ae58b57f26 Mon Sep 17 00:00:00 2001 From: Zelda Hessler Date: Tue, 10 Oct 2023 13:01:18 -0500 Subject: [PATCH 6/9] appease mdbook --- .../rfc0037_retry_classifier_customization.md | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/design/src/rfcs/rfc0037_retry_classifier_customization.md b/design/src/rfcs/rfc0037_retry_classifier_customization.md index 1e451d0060..e6d4fa3f66 100644 --- a/design/src/rfcs/rfc0037_retry_classifier_customization.md +++ b/design/src/rfcs/rfc0037_retry_classifier_customization.md @@ -48,7 +48,7 @@ they'll always run in a consistent order. Classifiers are each run in turn by the retry strategy: -```rust +```rust,ignore pub fn run_classifiers_on_ctx( classifiers: impl Iterator, ctx: &InterceptorContext, @@ -98,7 +98,7 @@ implemented, users will be able to define and set their own classifiers. ### Defining a custom classifier -```rust +```rust,ignore #[derive(Debug)] struct CustomRetryClassifier; @@ -142,7 +142,7 @@ system is private and inaccessible to users. Instead, users may set the priority of classifiers relative to one another with the `with_lower_priority_than` and `with_higher_priority_than` methods: -```rust +```rust,ignore impl RetryClassifierPriority { /// Create a new `RetryClassifierPriority` with lower priority than the given priority. pub fn with_lower_priority_than(other: Self) -> Self { ... } @@ -152,11 +152,11 @@ impl RetryClassifierPriority { } ``` -For example, if it was important for our `CustomRetryClassifer` in the previous +For example, if it was important for our `CustomRetryClassifier` in the previous example to run *before* the default `HttpStatusCodeClassifier`, a user would -define the `CustomRetryClassifer` priority like this: +define the `CustomRetryClassifier` priority like this: -```rust +```rust,ignore impl ClassifyRetry for CustomRetryClassifier { fn priority(&self) -> RetryClassifierPriority { RetryClassifierPriority::run_before(RetryClassifierPriority::http_status_code_classifier()) @@ -174,7 +174,7 @@ set a retry priority relative to an existing retry priority. Retry classifiers communicate to the retry strategy by emitting `RetryAction`s: -```rust +```rust,ignore /// The result of running a [`ClassifyRetry`] on a [`InterceptorContext`]. #[non_exhaustive] #[derive(Clone, Eq, PartialEq, Debug, Default)] @@ -197,7 +197,7 @@ pub enum RetryAction { When a retry is indicated by a classifier, the action will contain a `RetryReason`: -```rust +```rust,ignore /// The reason for a retry. #[non_exhaustive] #[derive(Clone, Eq, PartialEq, Debug)] @@ -212,11 +212,11 @@ pub enum RetryReason { } ``` -*NOTE: `RetryReason` currently only has a single variant but it's defined as an `enum` for [forward compatibility] purposes.* +*NOTE: `RetryReason` currently only has a single variant, but it's defined as an `enum` for [forward compatibility] purposes.* `RetryAction`'s `impl` defines several convenience methods: -```rust +```rust,ignore impl RetryAction { /// Create a new `RetryAction` indicating that a retry is necessary. pub fn retryable_error(kind: ErrorKind) -> Self { @@ -261,7 +261,7 @@ impl RetryAction { The interface for setting classifiers is very similar to the interface of settings interceptors: -```rust +```rust,ignore // All service configs support these setters. Operations support a nearly identical API. impl ServiceConfigBuilder { /// Add type implementing ClassifyRetry that will be used by the RetryStrategy @@ -303,7 +303,7 @@ Smithy clients have three classifiers enabled by default: one is encountered, returns `Some(RetryAction::Retry(ErrorKind::TransientError))`. Otherwise, returns `None`. Requires a parsed response. -- `HttpStatusCodeClassifier`: Checks the HTTP response's status code. By default +- `HttpStatusCodeClassifier`: Checks the HTTP response's status code. By default, this classifies `500`, `502`, `503`, and `504` errors as `Some(RetryAction::Retry(ErrorKind::TransientError))`. Otherwise, returns `None`. The list of retryable status codes may be customized when creating @@ -356,7 +356,7 @@ In order to implement this feature, we must: ### The `RetryClassifier` trait -```rust +```rust,ignore /// The result of running a [`ClassifyRetry`] on a [`InterceptorContext`]. #[non_exhaustive] #[derive(Clone, Eq, PartialEq, Debug)] @@ -411,7 +411,7 @@ expected order. - **A:** I think no, because of the added complexity. If we make them fallible then we'll have to decide what happens when classifiers fail. Do we skip them or does classification end? The retry strategy is responsible for - calling the classifiers so it be responsible for deciding how to handle a + calling the classifiers, so it be responsible for deciding how to handle a classifier error. I don't foresee a use case where an error returned by a classifier would be interpreted either by classifiers following the failed classifier or the retry strategy. From 8ed229a83be3352bc9c61a844e0714bf458aae51 Mon Sep 17 00:00:00 2001 From: Zelda Hessler Date: Tue, 10 Oct 2023 13:34:03 -0500 Subject: [PATCH 7/9] rename rfc file --- ...customization.md => rfc0038_retry_classifier_customization.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename design/src/rfcs/{rfc0037_retry_classifier_customization.md => rfc0038_retry_classifier_customization.md} (100%) diff --git a/design/src/rfcs/rfc0037_retry_classifier_customization.md b/design/src/rfcs/rfc0038_retry_classifier_customization.md similarity index 100% rename from design/src/rfcs/rfc0037_retry_classifier_customization.md rename to design/src/rfcs/rfc0038_retry_classifier_customization.md From bd1d909c9195dfca37c24ddacf76776ad215c518 Mon Sep 17 00:00:00 2001 From: Zelda Hessler Date: Tue, 10 Oct 2023 14:50:16 -0500 Subject: [PATCH 8/9] mark RFC as implemented --- .../rfc0038_retry_classifier_customization.md | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/design/src/rfcs/rfc0038_retry_classifier_customization.md b/design/src/rfcs/rfc0038_retry_classifier_customization.md index e6d4fa3f66..789205b3ba 100644 --- a/design/src/rfcs/rfc0038_retry_classifier_customization.md +++ b/design/src/rfcs/rfc0038_retry_classifier_customization.md @@ -1,6 +1,6 @@ # RFC: User-configurable retry classification -> Status: RFC +> Status: Implemented > > Applies to: client @@ -418,16 +418,16 @@ expected order. ## Changes checklist -- [ ] Add retry classifiers field and setters to `RuntimeComponents` and `RuntimeComponentsBuilder`. - - [ ] Add unit tests ensuring that classifier priority is respected by `RuntimeComponents::retry_classifiers`, especially when multiple layers of config are in play. -- [ ] Add codegen customization allowing users to set retry classifiers on service configs. -- [ ] Add codegen for setting default classifiers at the service level. - - [ ] Add integration tests for setting classifiers at the service level. -- [ ] Add codegen for settings default classifiers that require knowledge of operation error types at the operation level. - - [ ] Add integration tests for setting classifiers at the operation level. -- [ ] Implement retry classifier priority. - - [ ] Add unit tests for retry classifier priority. -- [ ] Update existing tests that would fail for lack of a retry classifier. +- [x] Add retry classifiers field and setters to `RuntimeComponents` and `RuntimeComponentsBuilder`. + - [x] Add unit tests ensuring that classifier priority is respected by `RuntimeComponents::retry_classifiers`, especially when multiple layers of config are in play. +- [x] Add codegen customization allowing users to set retry classifiers on service configs. +- [x] Add codegen for setting default classifiers at the service level. + - [x] Add integration tests for setting classifiers at the service level. +- [x] Add codegen for settings default classifiers that require knowledge of operation error types at the operation level. + - [x] Add integration tests for setting classifiers at the operation level. +- [x] Implement retry classifier priority. + - [x] Add unit tests for retry classifier priority. +- [x] Update existing tests that would fail for lack of a retry classifier. From 3be18e3c439b0f9f0cf6916ffa54f40868094641 Mon Sep 17 00:00:00 2001 From: Zelda Hessler Date: Tue, 10 Oct 2023 15:10:35 -0500 Subject: [PATCH 9/9] update the Default Classifiers section --- .../rfc0038_retry_classifier_customization.md | 36 ++++++++----------- 1 file changed, 15 insertions(+), 21 deletions(-) diff --git a/design/src/rfcs/rfc0038_retry_classifier_customization.md b/design/src/rfcs/rfc0038_retry_classifier_customization.md index 789205b3ba..ecbbbd3ad5 100644 --- a/design/src/rfcs/rfc0038_retry_classifier_customization.md +++ b/design/src/rfcs/rfc0038_retry_classifier_customization.md @@ -297,36 +297,30 @@ Smithy clients have three classifiers enabled by default: - `ModeledAsRetryableClassifier`: Checks for errors that are marked as retryable in the smithy model. If one is encountered, returns - `Some(RetryAction::Retry(ErrorKind))`. Otherwise, returns `None`. Requires a - parsed response. + `RetryAction::RetryIndicated`. Requires a parsed response. - `TransientErrorClassifier`: Checks for timeout, IO, and connector errors. If - one is encountered, returns - `Some(RetryAction::Retry(ErrorKind::TransientError))`. Otherwise, returns - `None`. Requires a parsed response. -- `HttpStatusCodeClassifier`: Checks the HTTP response's status code. By default, - this classifies `500`, `502`, `503`, and `504` errors as - `Some(RetryAction::Retry(ErrorKind::TransientError))`. Otherwise, returns - `None`. The list of retryable status codes may be customized when creating - this classifier with the `HttpStatusCodeClassifier::new_from_codes` method. + one is encountered, returns `RetryAction::RetryIndicated`. Requires a parsed + response. +- `HttpStatusCodeClassifier`: Checks the HTTP response's status code. By + default, this classifies `500`, `502`, `503`, and `504` errors as + `RetryAction::RetryIndicated`. The list of retryable status codes may be + customized when creating this classifier with the + `HttpStatusCodeClassifier::new_from_codes` method. -AWS clients enable the three smithy classifiers as well as two more by default: +AWS clients enable the three smithy classifiers as well as one more by default: - `AwsErrorCodeClassifier`: Checks for errors with AWS error codes marking them as either transient or throttling errors. If one is encountered, returns - `Some(RetryAction::Retry(ErrorKind))`. Otherwise, returns `None`. Requires a - parsed response. -- `AmzRetryAfterHeaderClassifier`: Checks the HTTP response for an - `x-amz-retry-after` header. If one is set, returns - `Some(RetryAction::RetryAfter(duration))` where duration is the header's - value. Otherwise, returns `None`. + `RetryAction::RetryIndicated`. Requires a parsed response. This classifier + will also check the HTTP response for an `x-amz-retry-after` header. If one is + set, then the returned `RetryAction` will include the explicit delay. The priority order of these classifiers is as follows: 1. *(highest priority)* `TransientErrorClassifier` -2. `AmzRetryAfterHeaderClassifier` -3. `ModeledAsRetryableClassifier` -4. `AwsErrorCodeClassifier` -5. *(lowest priority)* `HttpStatusCodeClassifier` +2. `ModeledAsRetryableClassifier` +3. `AwsErrorCodeClassifier` +4. *(lowest priority)* `HttpStatusCodeClassifier` The priority order of the default classifiers is not configurable. However, it's possible to wrap a default classifier in a newtype and set your desired priority