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.

Sign up for GitHub

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

Jump to bottom

Improve timeout config ergonomics and add SDK default timeouts #1740

Merged
merged 18 commits into from
Sep 27, 2022
Merged

Improve timeout config ergonomics and add SDK default timeouts #1740

merged 18 commits into from
Sep 27, 2022

Conversation

jdisanti
Copy link
Collaborator

@jdisanti jdisanti commented Sep 16, 2022 •
edited
Loading

Motivation and Context

This is work towards #256 for SDK production readiness. This PR:

  • Overhauls timeout configuration so that it more closely resembles retry configuration
  • Removes configs for timeouts that are not implemented and have no plan to be implemented
  • Removes non-standard env var/profile file timeout configuration values so that they won't be a roadblock for standardizing them across SDKs at a later date
  • Improves re-exports for runtime types so that customers are less likely to need to take direct dependencies on them
  • Refactors Smithy client building:
    • Smithy clients can no longer have their retry/timeout configs changed after creation. Previously, this would have allowed them to get into an invalid state since the sleep_impl validation takes place inside of the builder's build() method.
    • Tracking standard vs. non-standard retry policy is now handled better in the builder implementation
    • Made the builder's setter functions consistent with our other builders
  • Moves the code generator for CustomizableOperation into its own file
  • Makes it possible to code generate non-root modules
  • Renames the crate::customizable_operation module to crate::operation::customize
  • Re-exports types that can be used with operation customization inside of crate::operation::customize

Checklist

  • I have updated the AWS SDK examples
  • 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.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Velfi
Velfi approved these changes Sep 16, 2022
View reviewed changes
Copy link
Contributor

@Velfi Velfi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I left some comments but this otherwise looks great. Thanks for simplifying all this so much. I love to see changes that delete more code than they add 😄

...are/amazon/smithy/rust/codegen/client/smithy/customizations/ResiliencyConfigCustomization.kt Show resolved Hide resolved
jdisanti
jdisanti commented Sep 16, 2022
View reviewed changes
aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/AwsFluentClientDecorator.kt Outdated
Comment on lines 187 to 190
let mut builder = #{aws_smithy_client}::Builder::dyn_https()
.middleware(#{DynMiddleware}::new(#{Middleware}::new()));
builder.set_retry_config(retry_config.into());
builder.set_timeout_config(timeout_config);
// the builder maintains a try-state. To avoid suppressing the warning when sleep is unset,
// only set it if we actually have a sleep impl.
if let Some(sleep_impl) = sleep_impl {
builder.set_sleep_impl(Some(sleep_impl));
}
.middleware(#{DynMiddleware}::new(#{Middleware}::new()))
.retry_config(retry_config.into())
.timeout_config(timeout_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.

Note for myself: I started integration testing the connect timeout, and that revealed that the timeout_config here gets discarded since dyn_https() creates the connector and has no consideration of the timeout_config value at all.

This is another one of those weird situations with the aws_smithy_client::Builder similar to retry_config where you can configure a default implementation, or you can replace that implementation wholesale. I think we need to redesign this builder so that these conflicting builder methods are no longer possible. If you replace an implementation, then it shouldn't be possible to set configuration for the default.

@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.

This was referenced Sep 21, 2022
Closed
Closed
@jdisanti jdisanti marked this pull request as ready for review September 22, 2022 00:58
@jdisanti jdisanti requested review from a team as code owners September 22, 2022 00:58
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti jdisanti mentioned this pull request Sep 23, 2022
Closed
hlbarber
hlbarber approved these changes Sep 26, 2022
View reviewed changes
Copy link
Contributor

@hlbarber hlbarber left a comment

Choose a reason for hiding this comment

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

Server side looks good.

Velfi
Velfi reviewed Sep 26, 2022
View reviewed changes
aws/rust-runtime/aws-config/src/ecs.rs
Comment on lines +70 to +71
const DEFAULT_READ_TIMEOUT: Duration = Duration::from_secs(5);
const DEFAULT_CONNECT_TIMEOUT: Duration = Duration::from_secs(2);
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be nice if all these defaults were documented in one place in our FAQ or something. Or perhaps they could be imported from a crate of constants?

Copy link
Contributor

Choose a reason for hiding this comment

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

On second thought, I think defining these settings in the module where they are immediately relevant is the right choice, disregard my above comment.

Velfi
Velfi reviewed Sep 26, 2022
View reviewed changes
rust-runtime/aws-smithy-client/src/builder.rs
use crate::hyper_ext::Adapter as HyperAdapter;

#[cfg(feature = "rustls")]
impl<M, R> Builder<(), M, R> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is so clever and cool; Love it.

@jdisanti jdisanti mentioned this pull request Sep 27, 2022
Closed
2 tasks
@jdisanti
Copy link
Collaborator Author

Opened #1771 to track fixing the examples.

@jdisanti jdisanti enabled auto-merge (squash) September 27, 2022 00:44
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti jdisanti merged commit 8e0b440 into main Sep 27, 2022
@jdisanti jdisanti deleted the jdisanti-default-timeouts branch September 27, 2022 01:15
@jdisanti jdisanti mentioned this pull request Sep 27, 2022
Closed
7 tasks
@jdisanti jdisanti mentioned this pull request Oct 5, 2022
Merged
2 tasks
@pH14 pH14 mentioned this pull request Oct 7, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Velfi Velfi Velfi approved these changes

@hlbarber hlbarber hlbarber approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jdisanti @Velfi @hlbarber