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.
jdisanti
force-pushed
the
jdisanti-connector-switchover
branch
from
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. |
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.
maybe we should add derive(Hash) to settings to make this cleaner
There was a problem hiding this comment.
I was worried that could tie our hands in some way.
There was a problem hiding this comment.
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.
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.
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.
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.
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.
Maybe I'm not understanding your concern correctly, but I don't think I agree.
There was a problem hiding this comment.
if you call SharedAsyncSleep::new and pass it something that's already a SharedAsyncSleep, you won't take advantage of the magic, right?
|
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
DynConnectorandHttpConnectorfrom theaws_smithy_clientcrate with their counterparts in aws-smithy-runtime-api and aws-smithy-runtime. It also introducesHttpClientto make HTTP connector selection a smoother process, and adds a runtime plugin that configures a defaultHttpClientso thatConfigdoesn't have to do that.The
DnsServicefrom 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.