Enable stalled stream protection for uploads behind new behavior version #3527

jdisanti · 2024-03-27T23:32:13Z

This PR creates a new behavior version that enables stalled stream protection for upload streams by default.

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 · 2024-03-28T00:06:35Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

rcoh

lgtm, couple of inline comments

rcoh · 2024-03-28T14:34:15Z

rust-runtime/aws-smithy-runtime/src/client/defaults.rs

        .with_config(layer("default_stalled_stream_protection_config", |layer| {
-            layer.store_put(
-                StalledStreamProtectionConfig::enabled()
-                    // TODO(https://github.com/smithy-lang/smithy-rs/issues/3510): enable behind new behavior version
-                    .upload_enabled(false)
-                    .grace_period(Duration::from_secs(5))
-                    .build(),
-            );
+            let mut config =
+                StalledStreamProtectionConfig::enabled().grace_period(Duration::from_secs(5));
+            // Before v2024_03_28, upload streams did not have stalled stream protection by default
+            if !behavior_version.is_at_least(BehaviorVersion::v2024_03_28()) {
+                config = config.upload_enabled(false);
+            }
+            layer.store_put(config.build());


for debuggability, I wonder if we should alter the name of the plugin.

I would probably make two separate plugins that we select based on BMV instead, may be clearer and less likely to accidentally regress. That said, I don't have strong feelings.

rcoh · 2024-03-28T14:35:26Z

aws/sdk/integration-tests/s3/tests/stalled-stream-protection.rs

+        // Stalled stream protection MUST BE enabled by default. Do not configure it explicitly.
        .credentials_provider(Credentials::for_tests())
        .region(Region::new("us-east-1"))
        .endpoint_url(format!("http://{server_addr}"))
-        // TODO(https://github.com/smithy-lang/smithy-rs/issues/3510): make stalled stream protection enabled by default with BMV and remove this line
-        .stalled_stream_protection(StalledStreamProtectionConfig::enabled().build())
        .build();


should we copy one of these tests, use the older BMV, and validate that stalled stream upload protection is not enabled?

rcoh · 2024-03-28T14:36:41Z

CHANGELOG.next.toml

@@ -70,3 +70,15 @@ message = "Stalled stream protection on downloads will now only trigger if the u
 references = ["smithy-rs#3485"]
 meta = { "breaking" = false, "tada" = false, "bug" = true }
 author = "jdisanti"
+
+[[smithy-rs]]
+message = "Stalled stream protection on uploads is now enabled by default behind `BehaviorVersion::v2024_03_28()`. If you're using `BehaviorVersion::latest()`, you will get this change automatically by updating."


Suggested change

message = "Stalled stream protection on uploads is now enabled by default behind `BehaviorVersion::v2024_03_28()`. If you're using `BehaviorVersion::latest()`, you will get this change automatically by updating."

message = "Stalled stream protection on uploads is now enabled by default behind `BehaviorVersion::v2024_03_28()`. If you're using `BehaviorVersion::latest()`, you will get this change automatically by running `cargo update`. Updating your SDK is not necessary, this change will happen when a new version of the client libraries are consumed."

aajtodd · 2024-04-03T14:55:40Z

rust-runtime/aws-smithy-runtime-api/src/client/behavior_version.rs

 ///
 /// Over time, new best-practice behaviors are introduced. However, these behaviors might not be
 /// backwards compatible. For example, a change which introduces new default timeouts or a new
 /// retry-mode for all operations might be the ideal behavior but could break existing applications.
-#[derive(Clone)]
+#[derive(Copy, Clone, PartialEq)]
 pub struct BehaviorVersion {


We should probably add a section to the developer guide on behavior versions, what they are and guidance on how to choose, and what changed in each.

It's there - https://docs.aws.amazon.com/sdk-for-rust/latest/dg/client.html#client-behavior

github-actions · 2024-04-10T16:55:35Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

## Motivation and Context   Original issue: #3427 RFC: #3544 ## Description  Fixes the `SharedCredentialsProvider` and `SharedTokenProvider` to re-use a consistent cache partition by default. `SdkConfig` does not create an identity cache by default still, that will need to be a follow on PR that gates it behind a new behavior version. Ideally we do that with the stalled stream protection work in #3527 ## Testing    Added new unit and integration tests. ## Checklist  - [X] I have updated `CHANGELOG.next.toml` if I made changes to the smithy-rs codegen or 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._

…loads

github-actions · 2024-04-12T23:49:01Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

…loads

github-actions · 2024-04-23T23:09:58Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

github-actions · 2024-04-24T18:28:24Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

github-actions · 2024-04-24T19:04:11Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

## Motivation and Context This PR is the combination of two previously reviewed PRs that both enable new behaviors behind a new Behavior Major Version (BMV): * #3527 * #3578 ## Description * Enables stalled stream protection by default on latest behavior version. * Enables creation of a default identity cache. ## Testing All testing done on prior PRs. See for details. ## Checklist  - [x] I have updated `CHANGELOG.next.toml` if I made changes to the smithy-rs codegen or runtime crates - [x] 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._ --------- Co-authored-by: John DiSanti <jdisanti@amazon.com> Co-authored-by: ysaito1001 <awsaito@amazon.com>

This PR is the combination of two previously reviewed PRs that both enable new behaviors behind a new Behavior Major Version (BMV): * smithy-lang#3527 * smithy-lang#3578 * Enables stalled stream protection by default on latest behavior version. * Enables creation of a default identity cache. All testing done on prior PRs. See for details.  - [x] I have updated `CHANGELOG.next.toml` if I made changes to the smithy-rs codegen or runtime crates - [x] 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._ --------- Co-authored-by: John DiSanti <jdisanti@amazon.com> Co-authored-by: ysaito1001 <awsaito@amazon.com>

## Motivation and Context This PR is the combination of two previously reviewed PRs that both enable new behaviors behind a new Behavior Major Version (BMV): * smithy-lang/smithy-rs#3527 * smithy-lang/smithy-rs#3578 ## Description * Enables stalled stream protection by default on latest behavior version. * Enables creation of a default identity cache. ## Testing All testing done on prior PRs. See for details. ## Checklist  - [x] I have updated `CHANGELOG.next.toml` if I made changes to the smithy-rs codegen or runtime crates - [x] 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._ --------- Co-authored-by: John DiSanti <jdisanti@amazon.com> Co-authored-by: ysaito1001 <awsaito@amazon.com>

jdisanti added the needs-sdk-review label Mar 27, 2024

jdisanti marked this pull request as ready for review March 27, 2024 23:35

jdisanti requested review from a team as code owners March 27, 2024 23:35

jdisanti mentioned this pull request Mar 27, 2024

Enable upload stalled stream protection behind new BMV #3510

Open

4 tasks

rcoh approved these changes Mar 28, 2024

View reviewed changes

jdisanti added the blocked label Mar 28, 2024

aajtodd reviewed Apr 3, 2024

View reviewed changes

aajtodd mentioned this pull request Apr 5, 2024

rfc43: fix identity cache partition #3566

Merged

1 task

jdisanti added 2 commits April 10, 2024 09:36

Enable stalled stream protection for uploads behind new behavior version

d9d3a2f

Add changelog entries

477d1d5

jdisanti force-pushed the jdisanti-bmv-stall-uploads branch from e6ee189 to 477d1d5 Compare April 10, 2024 16:37

jdisanti removed the blocked label Apr 12, 2024

jdisanti added 3 commits April 12, 2024 16:20

Merge remote-tracking branch 'origin/main' into jdisanti-bmv-stall-up…

e72da5e

…loads

Fix runtime crate version numbers after merge

df57d1e

Improve changelog message

3f3ac63

jdisanti added 2 commits April 23, 2024 15:49

Merge remote-tracking branch 'origin/main' into jdisanti-bmv-stall-up…

6d25692

…loads

Fix deprecation error

4dac882

Fix another deprecation error

f7731ba

Fix aws-smithy-runtime version number after release

a21b6d9

jdisanti changed the base branch from main to behavior-version-2024-03-28 April 24, 2024 19:20

jdisanti merged commit 510ce11 into behavior-version-2024-03-28 Apr 24, 2024
44 checks passed

jdisanti deleted the jdisanti-bmv-stall-uploads branch April 24, 2024 19:20

aajtodd mentioned this pull request Apr 30, 2024

behavior version 2024-03-28 #3617

Merged

2 tasks

Velfi removed the needs-sdk-review label May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable stalled stream protection for uploads behind new behavior version #3527

Enable stalled stream protection for uploads behind new behavior version #3527

jdisanti commented Mar 27, 2024 •

edited

github-actions bot commented Mar 28, 2024

rcoh left a comment

rcoh Mar 28, 2024

rcoh Mar 28, 2024

rcoh Mar 28, 2024

aajtodd Apr 3, 2024

jdisanti Apr 3, 2024 •

edited

github-actions bot commented Apr 10, 2024

github-actions bot commented Apr 12, 2024

github-actions bot commented Apr 23, 2024

github-actions bot commented Apr 24, 2024

github-actions bot commented Apr 24, 2024

	message = "Stalled stream protection on uploads is now enabled by default behind `BehaviorVersion::v2024_03_28()`. If you're using `BehaviorVersion::latest()`, you will get this change automatically by updating."
	message = "Stalled stream protection on uploads is now enabled by default behind `BehaviorVersion::v2024_03_28()`. If you're using `BehaviorVersion::latest()`, you will get this change automatically by running `cargo update`. Updating your SDK is not necessary, this change will happen when a new version of the client libraries are consumed."

Enable stalled stream protection for uploads behind new behavior version #3527

Enable stalled stream protection for uploads behind new behavior version #3527

Conversation

jdisanti commented Mar 27, 2024 • edited

github-actions bot commented Mar 28, 2024

rcoh left a comment

Choose a reason for hiding this comment

rcoh Mar 28, 2024

Choose a reason for hiding this comment

rcoh Mar 28, 2024

Choose a reason for hiding this comment

rcoh Mar 28, 2024

Choose a reason for hiding this comment

aajtodd Apr 3, 2024

Choose a reason for hiding this comment

jdisanti Apr 3, 2024 • edited

Choose a reason for hiding this comment

github-actions bot commented Apr 10, 2024

github-actions bot commented Apr 12, 2024

github-actions bot commented Apr 23, 2024

github-actions bot commented Apr 24, 2024

github-actions bot commented Apr 24, 2024

jdisanti commented Mar 27, 2024 •

edited

jdisanti Apr 3, 2024 •

edited