-
Notifications
You must be signed in to change notification settings - Fork 227
Enable retries by default #4454
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
base: main
Are you sure you want to change the base?
Conversation
|
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. |
f7a4014 to
fc83a75
Compare
rcoh
left a comment
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.
Test failure is real, need to fix the test or pin the behavior version
I would also appreciate a few tests
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
8b3dcd0 to
8b014c8
Compare
|
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. |
ada4678 to
796622b
Compare
|
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. |
c82874c to
f02053c
Compare
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
f02053c to
0f588c2
Compare
|
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. |
|
Thinking about this more, I wonder if we should enable retries for AWS SDK clients ONLY by default (and not for non-SDK smithy clients). |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
aws/codegen-aws-sdk/src/main/kotlin/software/amazon/smithy/rustsdk/AwsFluentClientDecorator.kt
Show resolved
Hide resolved
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
61a604b to
c6c51da
Compare
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
c6c51da to
929a051
Compare
|
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. |
c2d8eca to
e43bec8
Compare
e43bec8 to
fb0de85
Compare
rcoh
left a comment
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.
Logic is backwards. This is important.
aws/codegen-aws-sdk/src/test/kotlin/software/amazon/smithy/rustsdk/TimeoutConfigMergingTest.kt
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. |
771078c to
61347dc
Compare
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
| } | ||
|
|
||
| #[test] | ||
| fn test_client_with_new_behavior_version_builds_successfully() { |
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.
These test descriptions are too generic, we have multiple behavior versions and will add more in the future. This should be named according to what it is testing more specifically (e.g. test_default_retry_bmbv_2025_08_07 or similar)
|
|
||
| #[tokio::test] | ||
| #[expect(deprecated)] | ||
| async fn test_connect_timeout_enabled_by_default_with_new_behavior_version() { |
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.
same comment on naming
| let behavior_version = params | ||
| .behavior_version | ||
| .unwrap_or_else(BehaviorVersion::latest); | ||
| let retry_partition = RetryPartition::new(default_partition_name); |
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.
let retry_partition = RetryPartition::new(params.retry_partition.expect("retry partition name is required")
Fixes awslabs/aws-sdk-rust#1389
Fixes awslabs/aws-sdk-rust#1393
Problem
When users create a client with
Config::builder().build(), retries are disabled by default. This is confusing because our docs say retries are enabled by default, andaws_config::load_from_env()does enable them. Users expect retries to just work.Solution
Enable
RetryConfig::standard()and a 3.1s connection timeout by default forBehaviorVersion::v2025_01_17()and later. Older behavior versions keep the current behavior for backward compatibility.Changes
default_retry_config_plugin()to enable retries for new behavior versionsdefault_timeout_config_plugin()to add 3.1s connection timeout.send()docs to clarify when retries are enabledDocumentation Updates Needed
The AWS Developer Guide at https://docs.aws.amazon.com/sdk-for-rust/latest/dg/retries.html needs to be updated to reflect this change.
Checklist
cc @rcoh @aajtodd