Fix native Smithy client retry and retry response errors #1717

jdisanti · 2022-09-07T23:02:16Z

Motivation and Context

This PR addresses awslabs/aws-sdk-rust#303 and #1715.

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.

github-actions · 2022-09-07T23:13:10Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python

A new doc preview is ready to view.

github-actions · 2022-09-07T23:27:53Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python

A new doc preview is ready to view.

jdisanti · 2022-09-08T00:27:12Z

Need to decide what to do about the "retry policy" to "response classifier" rename. The "response classifier" name is a lot more accurate, but the rename goes deep since the operation retry_policy and with_retry_policy functions will also need to be renamed, and examples fixed:
https://github.com/awsdocs/aws-doc-sdk-examples/blob/3b8c70b319882a40a03aedfcd5383b6851c9dd76/rust_dev_preview/dynamodb/src/bin/movies.rs#L302-L319

That example also mentions "retry every second until the table is ready" in its comments, but I highly doubt that's what's actually happening. It's probably using exponential backoff.

rcoh · 2022-09-08T01:37:05Z

That example also mentions "retry every second until the table is ready" in its comments, but I highly doubt that's what's actually happening. It's probably using exponential backoff.

It's using RetryKind::Explicit(Duration::from_secs(1)), so that is accurate at least.

I'd recommend doing the rename, adding new methods, deprecating the old ones, and deleting them in a future release to simplify coordination

Velfi

😍

Velfi · 2022-09-08T14:32:22Z

CHANGELOG.next.toml

@@ -186,3 +186,21 @@ message = "Smithy IDL v2 mixins are now supported"
 references = ["smithy-rs#1680"]
 meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "all"}
 author = "ogudavid"
+
+[[smithy-rs]]
+message = "Generated clients now retry transient errors without replacing the retry policy."


Suggested change

message = "Generated clients now retry transient errors without replacing the retry policy."

message = "White-label smithy clients now have a default retry policy and will retry requests returning a transient HTTP error."

White label/adhoc AWS clients already had retry. This is specifically fixing for Smithy clients. I think this should be clear by it being a smithy-rs entry.

aws/rust-runtime/aws-http/src/retry.rs

Velfi · 2022-09-08T14:36:58Z

aws/rust-runtime/aws-http/src/retry.rs

+pub struct AwsResponseClassifier;
+
+impl AwsResponseClassifier {
+    /// Create an `AwsResponseClassifier` with the default set of known error & status codes


It'd be cool if we could link to code or docs showing what errors get retried

If there is a way to link to the private constants with rustdoc, then I could add this pretty easily. I'm reluctant to duplicate the list in the documentation since it will get out of date.

aws/rust-runtime/aws-http/src/retry.rs

...kotlin/software/amazon/smithy/rust/codegen/smithy/generators/client/FluentClientGenerator.kt

Velfi · 2022-09-08T14:53:38Z

@jdisanti Will you be submitting an examples update PR as well?

…se-errors

github-actions · 2022-09-09T22:31:28Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python

A new doc preview is ready to view.

jdisanti · 2022-09-09T23:09:18Z

@jdisanti Will you be submitting an examples update PR as well?

Yeah.

github-actions · 2022-09-09T23:20:45Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python

A new doc preview is ready to view.

jdisanti · 2022-09-09T23:57:37Z

Examples fixed in awsdocs/aws-doc-sdk-examples#3593

github-actions · 2022-09-10T00:37:38Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python

A new doc preview is ready to view.

aws/rust-runtime/aws-http/src/retry.rs

aws/sdk/integration-tests/dynamodb/tests/movies.rs

Velfi · 2022-09-12T19:07:04Z

rust-runtime/aws-smithy-http-tower/src/lib.rs

@@ -102,7 +103,10 @@ mod tests {
        });

        let mut svc = ServiceBuilder::new()
-            .layer(ParseResponseLayer::<TestParseResponse, ()>::new())
+            .layer(ParseResponseLayer::<
+                TestParseResponse,


Consider calling this "TestResponseHandler" or, instead, changing the verbiage for "response handler" to "response parser"

I think the name is actually fine considering it implements ParseStrictResponse. I think handler might actually be the out of place name that should potentially get fixed in other places.

Co-authored-by: Zelda Hessler <zhessler@amazon.com>

…se-errors

github-actions · 2022-09-12T23:16:02Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python

A new doc preview is ready to view.

github-actions · 2022-09-14T20:12:19Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python

A new doc preview is ready to view.

jdisanti added 2 commits September 7, 2022 16:00

Fix retry for native Smithy clients

27a563e

Treat SdkError::ResponseError as a retryable transient failure

323758b

jdisanti requested a review from a team as a code owner September 7, 2022 23:02

Update changelog

b9b778a

jdisanti added the needs-sdk-review label Sep 7, 2022

Velfi approved these changes Sep 8, 2022

View reviewed changes

Velfi added waiting-on-author and removed needs-sdk-review labels Sep 8, 2022

jdisanti added 5 commits September 9, 2022 14:20

Rename ClassifyResponse to ClassifyRetry

a001b02

Rename 'policy' to 'classifier' in AWS SDK public API

ff771d5

Rename AwsResponseClassifier to AwsResponseRetryClassifier

01eae51

Incorporate feedback

c54cfc1

Merge remote-tracking branch 'origin/main' into jdisanti-retry-respon…

92cebfe

…se-errors

Fix codegen-client tests

02db8d7

jdisanti mentioned this pull request Sep 9, 2022

Simplify waiting logic in Rust DynamoDB examples awsdocs/aws-doc-sdk-examples#3593

Merged

2 tasks

Fix the SDK integration tests

ab6f6cf

jdisanti mentioned this pull request Sep 10, 2022

SDK Production Readiness #1692

Closed

28 tasks

jdisanti added needs-sdk-review and removed waiting-on-author labels Sep 12, 2022

Velfi reviewed Sep 12, 2022

View reviewed changes

aws/rust-runtime/aws-http/src/retry.rs Outdated Show resolved Hide resolved

Velfi reviewed Sep 12, 2022

View reviewed changes

aws/sdk/integration-tests/dynamodb/tests/movies.rs Outdated Show resolved Hide resolved

Velfi reviewed Sep 12, 2022

View reviewed changes

Velfi approved these changes Sep 12, 2022

View reviewed changes

Fix comment

ab64eb4

Co-authored-by: Zelda Hessler <zhessler@amazon.com>

jdisanti removed the needs-sdk-review label Sep 12, 2022

jdisanti added 2 commits September 12, 2022 16:03

Improve doc comment

9bf1fe2

Merge remote-tracking branch 'origin/main' into jdisanti-retry-respon…

c1e5350

…se-errors

Merge branch 'main' into jdisanti-retry-response-errors

5ae896e

jdisanti enabled auto-merge (squash) September 14, 2022 20:02

jdisanti merged commit e6177b3 into main Sep 14, 2022

jdisanti deleted the jdisanti-retry-response-errors branch September 14, 2022 20:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix native Smithy client retry and retry response errors #1717

Fix native Smithy client retry and retry response errors #1717

jdisanti commented Sep 7, 2022 •

edited

Loading

github-actions bot commented Sep 7, 2022

github-actions bot commented Sep 7, 2022

jdisanti commented Sep 8, 2022 •

edited

Loading

rcoh commented Sep 8, 2022

Velfi left a comment

Velfi Sep 8, 2022

jdisanti Sep 9, 2022

Velfi Sep 8, 2022

jdisanti Sep 9, 2022

Velfi commented Sep 8, 2022

github-actions bot commented Sep 9, 2022

jdisanti commented Sep 9, 2022

github-actions bot commented Sep 9, 2022

jdisanti commented Sep 9, 2022

github-actions bot commented Sep 10, 2022

Velfi Sep 12, 2022

jdisanti Sep 12, 2022

github-actions bot commented Sep 12, 2022

github-actions bot commented Sep 14, 2022

	message = "Generated clients now retry transient errors without replacing the retry policy."
	message = "White-label smithy clients now have a default retry policy and will retry requests returning a transient HTTP error."

Fix native Smithy client retry and retry response errors #1717

Fix native Smithy client retry and retry response errors #1717

Conversation

jdisanti commented Sep 7, 2022 • edited Loading

Motivation and Context

Checklist

github-actions bot commented Sep 7, 2022

github-actions bot commented Sep 7, 2022

jdisanti commented Sep 8, 2022 • edited Loading

rcoh commented Sep 8, 2022

Velfi left a comment

Choose a reason for hiding this comment

Velfi Sep 8, 2022

Choose a reason for hiding this comment

jdisanti Sep 9, 2022

Choose a reason for hiding this comment

Velfi Sep 8, 2022

Choose a reason for hiding this comment

jdisanti Sep 9, 2022

Choose a reason for hiding this comment

Velfi commented Sep 8, 2022

github-actions bot commented Sep 9, 2022

jdisanti commented Sep 9, 2022

github-actions bot commented Sep 9, 2022

jdisanti commented Sep 9, 2022

github-actions bot commented Sep 10, 2022

Velfi Sep 12, 2022

Choose a reason for hiding this comment

jdisanti Sep 12, 2022

Choose a reason for hiding this comment

github-actions bot commented Sep 12, 2022

github-actions bot commented Sep 14, 2022

jdisanti commented Sep 7, 2022 •

edited

Loading

jdisanti commented Sep 8, 2022 •

edited

Loading