-
Notifications
You must be signed in to change notification settings - Fork 193
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
Replace DynConnector
and HttpConnector
with HttpClient
#3011
Conversation
A new generated diff is ready to view.
A new doc preview is ready to view. |
This PR removes the last usages of `DynConnector` and `HttpClient` from the `aws_smithy_client` crate.
abbc13d
to
0af7920
Compare
I still need to write changelog entries and apply the |
...egen/src/main/kotlin/software/amazon/smithy/rustsdk/endpoints/OperationInputTestGenerator.kt
Show resolved
Hide resolved
...ware/amazon/smithy/rust/codegen/client/smithy/customizations/HttpConnectorConfigDecorator.kt
Show resolved
Hide resolved
.../software/amazon/smithy/rust/codegen/client/smithy/customizations/TimeSourceCustomization.kt
Outdated
Show resolved
Hide resolved
A new generated diff is ready to view.
A new doc preview is ready to view. |
The changelog entries and upgrade guidance are now in place. I think I incorporated all the feedback above, or explained why I'm punting on it. |
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.
I reviewed the new changes. It still looks good to me.
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.
effectively lgtm, but I expect that there will be one more pass
aws/sdk/integration-tests/no-default-features/tests/client-construction.rs
Show resolved
Hide resolved
{ | ||
fn http_connector( | ||
&self, | ||
settings: &HttpConnectorSettings, |
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.
maybe we should add derive(Hash)
to settings to make this cleaner
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 worried that could tie our hands in some way.
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 think this structure has to be Hash if we expect people to use it as a cache key
let settings = { | ||
let mut builder = HttpConnectorSettings::builder(); | ||
builder.set_connect_timeout(timeout_config.connect_timeout()); | ||
builder.set_read_timeout(timeout_config.read_timeout()); | ||
builder.build() | ||
}; | ||
let connector = http_client.http_connector(&settings, runtime_components); |
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.
operation-level timeouts didn't work before this right? Seems like we should add an integration test to capture that if we didn't already
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 don't know what gave you the impression operation timeouts weren't working. We have integration tests for all the varieties of timeouts: https://github.com/awslabs/smithy-rs/blob/0af7920eba8d8d0dac79035c862e1dde1b377afc/aws/sdk/integration-tests/s3/tests/timeouts.rs
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.
none of those tests change the HTTP connector timeouts for an individual operation (and I don't think we possibly could have supported that because we didn't have a cache keyed on http configuration)
mod async_impls { | ||
use aws_smithy_async::rt::sleep::{AsyncSleep, SharedAsyncSleep}; | ||
use aws_smithy_async::time::{SharedTimeSource, TimeSource}; | ||
impl_shared_conversions!(convert SharedAsyncSleep from AsyncSleep using SharedAsyncSleep::new); |
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.
side note: we should rename the new
methods to be something like new_from_unshared
so they don't get called by accident and we have a place to guide folks to the into_shared() method
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.
Maybe I'm not understanding your concern correctly, but I don't think I agree.
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 you call SharedAsyncSleep::new
and pass it something that's already a SharedAsyncSleep
, you won't take advantage of the magic, 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.
A fantastic piece of work, including docs and the upgrade guide ❤️ (for aws-smithy-runtime/src/client/http/test_util/*
I only skimmed through them)
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. |
This PR removes the last usages of
DynConnector
andHttpConnector
from theaws_smithy_client
crate with their counterparts in aws-smithy-runtime-api and aws-smithy-runtime. It also introducesHttpClient
to make HTTP connector selection a smoother process, and adds a runtime plugin that configures a defaultHttpClient
so thatConfig
doesn't have to do that.The
DnsService
from aws-config is also moved into aws-smithy-runtime and refactored to be consistent with the other configurable traits in the orchestrator since it will likely be used in the future for more than just the ECS credentials provider.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.