Skip to content
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

fix!: Retry config #1744

Merged
merged 6 commits into from
Sep 13, 2024
Merged

fix!: Retry config #1744

merged 6 commits into from
Sep 13, 2024

Conversation

sichanyoo
Copy link
Contributor

Issue #

Description of changes

  • Adds back the previously lost config maxAttempts that allows SDK users to configure number of retries the SDK should make without having to directly manipulate the retryStategyOptions config option.
  • Also: Fixes the config defaults provider and codegen for it, such that the maxAttempts and awsRetryMode config values set by the user actually get used when we resolve the default retryStrategyOptions config.

New/existing dependencies impact assessment, if applicable

Conventional Commits

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@sichanyoo sichanyoo requested a review from jbelkins September 13, 2024 19:48
Copy link
Contributor Author

@sichanyoo sichanyoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments for reviewers

@@ -99,17 +99,30 @@ public class AWSClientConfigDefaultsProvider {
return resolvedRetryMode!
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GH diff is being a bit wonky here. I just added private static func maxAttempts, and changed the function signature of func retryStrategyOptions to take in arguments (the top level config awsRetryMode and maxAttempts) at call site.

@@ -31,9 +31,19 @@ public protocol AWSDefaultClientConfiguration {

Copy link
Contributor Author

@sichanyoo sichanyoo Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adds back the lost maxAttempts option back to config. Let's welcome our poor option that finally found its way back home.

@@ -15,10 +15,13 @@ public class DefaultAWSClientPlugin: Plugin {
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that defaults provider for retry strategy options actually takes in user-exposed config now, let's use em.

@@ -67,7 +67,7 @@ class AWSHttpProtocolServiceClient(
ConfigProperty(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In initializers that take in arguments for awsRetryMode and maxAttempts, default value for retry strategy options should be defaults provider function call using those arguments.

In initializer that doesn't take in arguments for awsRetryMode and maxAttempts but needs to set retry strategy options value to something that isn't nil (e.g., the region initializer), we can't pass in any values to defaults provider method call.

@@ -39,5 +39,6 @@ class AWSDefaultClientConfiguration : ClientConfiguration {
{ it.format("\$N.retryMode()", AWSClientRuntimeTypes.Core.AWSClientConfigDefaultsProvider) },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adds the new config value in codegen side.

@sichanyoo sichanyoo merged commit 517490d into main Sep 13, 2024
29 checks passed
@sichanyoo sichanyoo deleted the fix/retry-config branch September 13, 2024 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants