-
Notifications
You must be signed in to change notification settings - Fork 188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
implement user-configurable retry classifiers #2977
Conversation
A new generated diff is ready to view.
A new doc preview is ready to view. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm reading this PR correctly, all (?) of our built-in classifiers are actually incorrect since they ignore the previous result causing retries to be blown away.
I think this is probably a hint that this interface as written is a tripping hazard / face rake and we should try to revise the implementor experience to make it easier to do the right thing—possibly by just altering the behavior of the main loop to not replace Some
with None
?
Can dig in more in the text based RFC
impl ClassifyRetry for CustomRetryClassifier { | ||
fn classify_retry( | ||
&self, | ||
_: &InterceptorContext, | ||
_: Option<RetryClassifierResult>, | ||
) -> Option<RetryClassifierResult> { | ||
*self.counter.lock().unwrap() += 1; | ||
|
||
Some(RetryClassifierResult::DontRetry) | ||
} | ||
|
||
fn name(&self) -> &'static str { | ||
"Custom Retry Classifier" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recommendation: Actually look at the request/response, & status code in this integration test to ensure that everything is properly wired.
aws/sdk/integration-tests/s3/tests/retry-classifier-customization.rs
Outdated
Show resolved
Hide resolved
...mazon/smithy/rust/codegen/client/smithy/customizations/RetryClassifierConfigCustomization.kt
Outdated
Show resolved
Hide resolved
pub fn retry_classifiers(&self) -> impl Iterator<Item = #{SharedRetryClassifier}> + '_ { | ||
self.runtime_components.retry_classifiers() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there an existing (customer usable) piece of code somewhere to classify a retry given a list of classifiers? Should this expose a list of classifiers or should it unify the list (effectively hiding the behavior that there is a list of these things)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose to return an iterator here because that's what we're doing for other 'list collected' things like interceptors.
rust-runtime/aws-smithy-runtime-api/src/client/retries/classifiers.rs
Outdated
Show resolved
Hide resolved
rust-runtime/aws-smithy-runtime-api/src/client/retries/classifiers.rs
Outdated
Show resolved
Hide resolved
rust-runtime/aws-smithy-runtime/src/client/connectors/connection_poisoning.rs
Outdated
Show resolved
Hide resolved
rust-runtime/aws-smithy-runtime/src/client/retries/classifiers.rs
Outdated
Show resolved
Hide resolved
rust-runtime/aws-smithy-runtime/src/client/retries/classifiers.rs
Outdated
Show resolved
Hide resolved
rust-runtime/aws-smithy-runtime/src/client/retries/classifiers.rs
Outdated
Show resolved
Hide resolved
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
…sifiers fix incorrect test JSON names add missing HttpStatusCodeClassifier to AssumeRoleProvider add section helper fn registerRetryClassifier to ServiceRuntimePluginSection.RegisterRuntimeComponents remove From<*Classifier> impls in favor of IntoShared
A new generated diff is ready to view.
A new doc preview is ready to view. |
rename RetryClassifierResult to RetryAction
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/HttpResponseChecksumDecorator.kt
Outdated
Show resolved
Hide resolved
pub fn push_retry_classifier(&mut self, retry_classifier: #{SharedRetryClassifier}) -> &mut Self { | ||
self.runtime_components.push_retry_classifier(retry_classifier); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IntoShared
will be merged into main
shortly:
pub fn push_retry_classifier(&mut self, retry_classifier: #{SharedRetryClassifier}) -> &mut Self { | |
self.runtime_components.push_retry_classifier(retry_classifier); | |
pub fn push_retry_classifier(&mut self, retry_classifier: impl #{RetryClassifier} + 'static) -> &mut Self { | |
self.runtime_components.push_retry_classifier(#{IntoShared}::into_shared(retry_classifier)); |
Although, why do we have both retry_classifier
and push_retry_classifier
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to mirror the Interceptor setters API.
rust-runtime/aws-smithy-runtime-api/src/client/retries/classifiers.rs
Outdated
Show resolved
Hide resolved
rust-runtime/aws-smithy-runtime-api/src/client/runtime_components.rs
Outdated
Show resolved
Hide resolved
rust-runtime/aws-smithy-runtime-api/src/client/runtime_components.rs
Outdated
Show resolved
Hide resolved
rust-runtime/aws-smithy-runtime/src/client/orchestrator/operation.rs
Outdated
Show resolved
Hide resolved
rust-runtime/aws-smithy-runtime/src/client/retries/strategy/fixed_delay.rs
Outdated
Show resolved
Hide resolved
""" | ||
/// Add type implementing [`ClassifyRetry`](#{ClassifyRetry}) that will be used by the | ||
/// [`RetryStrategy`](#{RetryStrategy}) to determine what responses should be retried. | ||
/// | ||
/// A retry classifier configured by this method will run according to its priority. | ||
/// | ||
/// ## Examples | ||
/// ```no_run | ||
/// ## ##[cfg(test)] | ||
/// ## mod tests { | ||
/// ## ##[test] | ||
/// ## fn example() { | ||
/// ## } | ||
/// ## } | ||
/// ``` | ||
pub fn retry_classifier(mut self, retry_classifier: impl #{ClassifyRetry} + 'static) -> Self { | ||
self.push_retry_classifier(#{SharedRetryClassifier}::new(retry_classifier)); | ||
self | ||
} | ||
|
||
/// Add a [`SharedRetryClassifier`](#{SharedRetryClassifier}) that will be used by the | ||
/// [`RetryStrategy`](#{RetryStrategy}) to determine what responses should be retried. | ||
/// | ||
/// A retry classifier configured by this method will run according to its priority. | ||
/// | ||
/// ## Examples | ||
/// ```no_run | ||
/// ## ##[cfg(test)] | ||
/// ## mod tests { | ||
/// ## ##[test] | ||
/// ## fn example() { | ||
/// ## } | ||
/// ## } | ||
/// ``` | ||
pub fn push_retry_classifier(&mut self, retry_classifier: #{SharedRetryClassifier}) -> &mut Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO add examples
refactor RetryAction remove previous_action arg from ClassifyRetry::classify_retry
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
fix incorrect XML in test remove unused dependency
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great. My only top-level thought to consider is that we may want to split DontCare
out from the list of RetryActions since that's something only a classifier cares about (and the orchestrator doesn't really).
Thinking something like:
enum RetryAction {
ActionIndicated(RetryReason),
NoActionIndicated
}
or something. This is definitely not critical though.
@@ -24,43 +25,49 @@ class RetryClassifierDecorator : ClientCodegenDecorator { | |||
baseCustomizations: List<OperationCustomization>, | |||
): List<OperationCustomization> = baseCustomizations + | |||
OperationRetryClassifiersFeature(codegenContext, operation) | |||
|
|||
override fun serviceRuntimePluginCustomizations( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no actual changes behaviorally to this file, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It used to mount all 5 classifiers. Now it only mounts the two that need knowledge of a specific error type to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scratch that, I combined the AWS classifiers because of the new "retry after" handling. This is now covered by the AWS Error code classifier
...egration-tests/s3/tests/data/request-information-headers/three-retries_and-then-success.json
Show resolved
Hide resolved
@@ -49,15 +52,17 @@ class RequiredCustomizations : ClientCodegenDecorator { | |||
baseCustomizations + | |||
MetadataCustomization(codegenContext, operation) + | |||
IdempotencyTokenGenerator(codegenContext, operation) + | |||
HttpChecksumRequiredGenerator(codegenContext, operation) | |||
HttpChecksumRequiredGenerator(codegenContext, operation) + | |||
RetryClassifierOperationCustomization(codegenContext, operation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how did this work before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, all retry classifiers were set per-operation in the RetryClassifierDecorator
. Now, they can be set at the config level AND the operation level.
...software/amazon/smithy/rust/codegen/client/smithy/generators/client/FluentClientGenerator.kt
Outdated
Show resolved
Hide resolved
|
||
/// Return the priority of this retry classifier. | ||
pub fn priority() -> RetryClassifierPriority { | ||
RetryClassifierPriority::modeled_as_retryable_classifier() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider renaming this something like RetryTraitClassifier
(non blocking)
rust-runtime/aws-smithy-runtime-api/src/client/retries/classifiers.rs
Outdated
Show resolved
Hide resolved
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
Read the RFC here
Motivation and Context
#2417
Description
Exactly what it says on the tin. I have a related RFC to publish that goes into more depth.
Testing
I wrote an integration test that ensures a custom retry classifier can be set and is called.
Checklist
CHANGELOG.next.toml
if I made changes to the smithy-rs codegen or runtime cratesCHANGELOG.next.toml
if I made changes to the AWS SDK, generated SDK code, or SDK runtime cratesBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.