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

Improve manual config experience for SDK retries and timeouts #1603

Merged
merged 16 commits into from
Sep 1, 2022

Conversation

jdisanti
Copy link
Collaborator

@jdisanti jdisanti commented Jul 29, 2022

Motivation and Context

This PR addresses awslabs/aws-sdk-rust#586 by:

  • Removing the Rust Default implementation for aws_config::RetryConfig and renaming RetryConfig::new to RetryConfig::standard
  • Adding integration tests for each of the possible retry/timeout configurations with/without an async sleep implementation
  • Improving the documentation around manual retry/timeout configuration
  • Adding a panic when an invalid configuration is detected

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

CHANGELOG.next.toml Outdated Show resolved Hide resolved
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@@ -130,6 +130,7 @@ async fn retry_test(sleep_impl: Arc<dyn AsyncSleep>) -> Result<(), Box<dyn std::
.credentials_provider(aws_types::credentials::SharedCredentialsProvider::new(
credentials,
))
.retry_config(RetryConfig::standard())
Copy link
Collaborator

Choose a reason for hiding this comment

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

need to figure out how this plays with aws-smithy-client::retry::Standard.config

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that's a good callout. I think we need to remove the retry policy from the client's generic args, since it really should be configured by the RetryConfig that feeds into Config rather than by the client builder. That way, the RetryMode in RetryConfig would set the policy instead.

If we want to retain the ability to customize the retry policy implementation completely for non-SDK Smithy clients, we could potentially add a Custom variant to RetryMode, but I don't think we want that for the SDK.

What are your thoughts on this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From discussion with @rcoh, it seems like removing the retry generic may be the best long term approach for simplifying this, but isn't currently possible due to tower::retry::Policy not being dyn-safe yet. This potential simplification should be punted to an RFC.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti
Copy link
Collaborator Author

Won't be able to fix the rest of CI until #1654 is merged, but I think this is otherwise ready for review.

@jdisanti jdisanti marked this pull request as ready for review August 23, 2022 01:37
@jdisanti jdisanti requested a review from a team as a code owner August 23, 2022 01:37
CHANGELOG.next.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@Velfi Velfi left a comment

Choose a reason for hiding this comment

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

I had 2 minor comments but otherwise this looks good.

P.S. we can probably delete TriState now; I think we were only using it for sleep impls

@jdisanti
Copy link
Collaborator Author

It looks like TriState is still used by timeouts.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti
Copy link
Collaborator Author

Posted a PR to fix the examples: awsdocs/aws-doc-sdk-examples#3526

@jdisanti jdisanti enabled auto-merge (squash) September 1, 2022 16:35
@github-actions
Copy link

github-actions bot commented Sep 1, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti jdisanti merged commit 353d81c into main Sep 1, 2022
@jdisanti jdisanti deleted the jdisanti-sleep-impl-confusion branch September 1, 2022 17:12
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.

3 participants