Port middleware connectors to the orchestrator #2970
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jdisanti
force-pushed
the
jdisanti-port-middleware-connectors
branch
2 times, most recently
from
bb69697
to
0c038ca
Compare
jdisanti
force-pushed
the
jdisanti-port-middleware-connectors
branch
from
0c038ca
to
cc98dfd
Compare
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
ysaito1001
approved these changes
Sep 1, 2023
Velfi
reviewed
Sep 5, 2023
rust-runtime/aws-smithy-runtime/src/client/connectors/test_util/dvr/record.rs
Outdated
Show resolved
Hide resolved
Velfi
approved these changes
Sep 5, 2023
Co-authored-by: Zelda Hessler <zhessler@amazon.com>
rcoh
reviewed
Sep 5, 2023
| author = "jdisanti" | ||
|
|
||
| [[smithy-rs]] | ||
| message = "TestConnection was renamed to EventConnector." |
There was a problem hiding this comment.
Although I agree the name is a little more descriptive, we should probably introduce some sort of table-of-contents of test connections
There was a problem hiding this comment.
Good call. Added in 17a518c
CHANGELOG.next.toml
Outdated
Comment on lines
105
to
128
|
|
||
| [[smithy-rs]] | ||
| message = "`aws_smithy_client::hyper_ext::Adapter` was moved/renamed to `aws_smithy_runtime::client::connectors::hyper_connector::HyperConnector`." | ||
| references = ["smithy-rs#2970"] | ||
| meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" } | ||
| author = "jdisanti" | ||
|
|
||
| [[smithy-rs]] | ||
| message = "Test connectors moved into `aws_smithy_runtime::client::connectors::test_util` behind the `test-util` feature." | ||
| references = ["smithy-rs#2970"] | ||
| meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" } | ||
| author = "jdisanti" | ||
|
|
||
| [[smithy-rs]] | ||
| message = "DVR's RecordingConnection and ReplayingConnection were renamed to RecordingConnector and ReplayingConnector respectively." | ||
| references = ["smithy-rs#2970"] | ||
| meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" } | ||
| author = "jdisanti" | ||
|
|
||
| [[smithy-rs]] | ||
| message = "TestConnection was renamed to EventConnector." | ||
| references = ["smithy-rs#2970"] | ||
| meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" } | ||
| author = "jdisanti" |
There was a problem hiding this comment.
can we add a doc explaining the difference between Connection and Connector?
There was a problem hiding this comment.
Talked about in 17a518c
| /// Boxed future used by [`HttpConnectorFuture`]. | ||
| pub type BoxFuture = Pin<Box<dyn StdFuture<Output = Result<HttpResponse, ConnectorError>> + Send>>; | ||
| /// Future for [`HttpConnector::call`]. | ||
| pub type HttpConnectorFuture = NowOrLater<Result<HttpResponse, ConnectorError>, BoxFuture>; |
There was a problem hiding this comment.
what if we introduce a new-type future instead? That seems like it would afford us more API evolution in the future
There was a problem hiding this comment.
Introduced in 8eb9923
| Ok(http::Response::builder() | ||
| .status(200) | ||
| .header("rp1", "1") | ||
| .body(SdkBody::empty()) | ||
| .unwrap()) | ||
| }) | ||
| })) | ||
| } | ||
| } | ||
|
|
||
| // CN2, the outer connector, calls the inner connector and adds the `rp2` header to the response | ||
| #[derive(Debug)] | ||
| struct CN2(SharedHttpConnector); |
There was a problem hiding this comment.
cn2 sounds like a protocol but I guess this is a test connector? maybe should be renamed Connector2 or something
There was a problem hiding this comment.
Renamed in 8eb9923
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
2 tasks
|
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. |
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 12, 2023
Draft pull request pending merge of #2970 ## Motivation and Context - awslabs/aws-sdk-rust#738 - awslabs/aws-sdk-rust#858 <!--- Why is this change required? What problem does it solve? --> <!--- If it fixes an open issue, please link to the issue here --> ## Description <!--- Describe your changes in detail --> This PR adds two additional classes of retries tested: 1. GO_AWAY: awslabs/aws-sdk-rust#738 2. REFUSED_STREAM: awslabs/aws-sdk-rust#858 I tested 1 by using the example helpfully provided. The fix for 2 is untested and difficult to reproduce but since my fix for 1 worked, I'm confident that we're detecting the correct error class here. ## Testing I used the example provided by the customer and validated the H2 GO_AWAY fix. ## Checklist <!--- If a checkbox below is not applicable, then please DELETE it rather than leaving it unchecked --> - [ ] 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._
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR ports all the connectors from the
aws-smithy-clientcrate intoaws-smithy-runtimeimplementing the newHttpConnectortrait. The old connectors are left in place for now, and follow up PRs will remove them as well as revise the generated configs to takeHttpConnectorimpls rather thanDynConnector.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.