Skip to content

Commit 1ddbc53

Browse files
authored
fix token bucket not being set for both standard and adaptive retry modes (#3964)
## Motivation and Context <!--- Why is this change required? What problem does it solve? --> <!--- If it fixes an open issue, please link to the issue here --> awslabs/aws-sdk-rust#1234 ## Description <!--- Describe your changes in detail --> PR adds a new interceptor registered as part of the default retry plugin components that ensures a token bucket is _always_ present and available to the retry strategy. The buckets are partitioned off the retry partition (which defaults to the service name and is already set by the default plugin). We use a `static` variable in the runtime for this which means that token buckets can and will apply to every single client that uses the same retry partition. The implementation tries to avoid contention on this new global lock by only consulting it if the retry partition is overridden after client creation. For AWS SDK clients I've updated the default retry partition clients are created with to include the region when set. Now the default partition for a client will be `{service}-{region}` (e.g. `sts-us-west-2`) rather than just the service name (e.g. `sts`). This partitioning is a little more granular and closer to what we want/expect as failures in one region should not cause throttling to another (and vice versa for success in one should not increase available quota in another). I also updated the implementation to follow the SEP a little more literally/closely as far as structure which fixes some subtle bugs. State is updated in one place and we ensure that the token bucket is always consulted (before the token bucket could be skipped in the case of adaptive retries returning a delay and the adaptive rate limit was updated in multiple branches). ## Testing <!--- Please describe in detail how you tested your changes --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> ## Checklist <!--- If a checkbox below is not applicable, then please DELETE it rather than leaving it unchecked --> - [x ] For changes to the smithy-rs codegen or runtime crates, I have created a changelog entry Markdown file in the `.changelog` directory, specifying "client," "server," or both in the `applies_to` key. - [ x] For changes to the AWS SDK, generated SDK code, or SDK runtime crates, I have created a changelog entry Markdown file in the `.changelog` directory, specifying "aws-sdk-rust" in the `applies_to` key. ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._
1 parent 0259f52 commit 1ddbc53

File tree

14 files changed

+525
-93
lines changed

14 files changed

+525
-93
lines changed

.changelog/1736370747.md

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
applies_to:
3+
- aws-sdk-rust
4+
- client
5+
authors:
6+
- aajtodd
7+
references:
8+
- aws-sdk-rust#1234
9+
breaking: false
10+
new_feature: false
11+
bug_fix: true
12+
---
13+
Fix token bucket not being set for standard and adaptive retry modes

aws/rust-runtime/Cargo.lock

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

aws/rust-runtime/aws-config/Cargo.lock

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/AwsFluentClientDecorator.kt

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import software.amazon.smithy.rust.codegen.core.rustlang.rustTemplate
2424
import software.amazon.smithy.rust.codegen.core.rustlang.writable
2525
import software.amazon.smithy.rust.codegen.core.smithy.RuntimeConfig
2626
import software.amazon.smithy.rust.codegen.core.smithy.RuntimeType
27+
import software.amazon.smithy.rust.codegen.core.smithy.RuntimeType.Companion.preludeScope
2728
import software.amazon.smithy.rust.codegen.core.smithy.RustCrate
2829
import software.amazon.smithy.rust.codegen.core.smithy.generators.LibRsCustomization
2930
import software.amazon.smithy.rust.codegen.core.smithy.generators.LibRsSection
@@ -58,6 +59,7 @@ class AwsFluentClientDecorator : ClientCodegenDecorator {
5859
listOf(
5960
AwsPresignedFluentBuilderMethod(codegenContext),
6061
AwsFluentClientDocs(codegenContext),
62+
AwsFluentClientRetryPartition(codegenContext),
6163
).letIf(codegenContext.serviceShape.id == ShapeId.from("com.amazonaws.s3#AmazonS3")) {
6264
it + S3ExpressFluentClientCustomization(codegenContext)
6365
},
@@ -166,3 +168,28 @@ private class AwsFluentClientDocs(private val codegenContext: ClientCodegenConte
166168
}
167169
}
168170
}
171+
172+
/**
173+
* Replaces the default retry partition for all operations to include the AWS region if set
174+
*/
175+
private class AwsFluentClientRetryPartition(private val codegenContext: ClientCodegenContext) : FluentClientCustomization() {
176+
override fun section(section: FluentClientSection): Writable {
177+
return when {
178+
section is FluentClientSection.BeforeBaseClientPluginSetup && usesRegion(codegenContext) -> {
179+
writable {
180+
rustTemplate(
181+
"""
182+
let default_retry_partition = match config.region() {
183+
Some(region) => #{Cow}::from(format!("{default_retry_partition}-{}", region)),
184+
None => #{Cow}::from(default_retry_partition),
185+
};
186+
""",
187+
*preludeScope,
188+
"Cow" to RuntimeType.Cow,
189+
)
190+
}
191+
}
192+
else -> emptySection
193+
}
194+
}
195+
}

aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/RegionDecorator.kt

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -83,13 +83,6 @@ class RegionDecorator : ClientCodegenDecorator {
8383
private val envKey = "AWS_REGION".dq()
8484
private val profileKey = "region".dq()
8585

86-
// Services that have an endpoint ruleset that references the SDK::Region built in, or
87-
// that use SigV4, both need a configurable region.
88-
private fun usesRegion(codegenContext: ClientCodegenContext) =
89-
codegenContext.getBuiltIn(AwsBuiltIns.REGION) != null ||
90-
ServiceIndex.of(codegenContext.model)
91-
.getEffectiveAuthSchemes(codegenContext.serviceShape).containsKey(SigV4Trait.ID)
92-
9386
override fun configCustomizations(
9487
codegenContext: ClientCodegenContext,
9588
baseCustomizations: List<ConfigCustomization>,
@@ -223,3 +216,14 @@ class RegionProviderConfig(codegenContext: ClientCodegenContext) : ConfigCustomi
223216
}
224217

225218
fun region(runtimeConfig: RuntimeConfig) = AwsRuntimeType.awsTypes(runtimeConfig).resolve("region")
219+
220+
/**
221+
* Test if region is used and configured for a model (and available on a service client).
222+
*
223+
* Services that have an endpoint ruleset that references the SDK::Region built in, or
224+
* that use SigV4, both need a configurable region.
225+
*/
226+
fun usesRegion(codegenContext: ClientCodegenContext) =
227+
codegenContext.getBuiltIn(AwsBuiltIns.REGION) != null ||
228+
ServiceIndex.of(codegenContext.model)
229+
.getEffectiveAuthSchemes(codegenContext.serviceShape).containsKey(SigV4Trait.ID)
Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
/*
2+
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
package software.amazon.smithy.rustsdk
6+
7+
import org.junit.jupiter.api.Test
8+
import software.amazon.smithy.rust.codegen.core.rustlang.CargoDependency
9+
import software.amazon.smithy.rust.codegen.core.rustlang.rustTemplate
10+
import software.amazon.smithy.rust.codegen.core.smithy.RuntimeType
11+
import software.amazon.smithy.rust.codegen.core.testutil.integrationTest
12+
import software.amazon.smithy.rust.codegen.core.testutil.tokioTest
13+
14+
class RetryPartitionTest {
15+
@Test
16+
fun `default retry partition`() {
17+
awsSdkIntegrationTest(SdkCodegenIntegrationTest.model) { ctx, rustCrate ->
18+
val rc = ctx.runtimeConfig
19+
val codegenScope =
20+
arrayOf(
21+
*RuntimeType.preludeScope,
22+
"capture_request" to RuntimeType.captureRequest(rc),
23+
"capture_test_logs" to
24+
CargoDependency.smithyRuntimeTestUtil(rc).toType()
25+
.resolve("test_util::capture_test_logs::capture_test_logs"),
26+
"Credentials" to
27+
AwsRuntimeType.awsCredentialTypesTestUtil(rc)
28+
.resolve("Credentials"),
29+
"Region" to AwsRuntimeType.awsTypes(rc).resolve("region::Region"),
30+
)
31+
32+
rustCrate.integrationTest("default_retry_partition") {
33+
tokioTest("default_retry_partition_includes_region") {
34+
val moduleName = ctx.moduleUseName()
35+
rustTemplate(
36+
"""
37+
let (_logs, logs_rx) = #{capture_test_logs}();
38+
let (http_client, _rx) = #{capture_request}(#{None});
39+
let client_config = $moduleName::Config::builder()
40+
.http_client(http_client)
41+
.region(#{Region}::new("us-west-2"))
42+
.credentials_provider(#{Credentials}::for_tests())
43+
.build();
44+
45+
let client = $moduleName::Client::from_conf(client_config);
46+
47+
let _ = client
48+
.some_operation()
49+
.send()
50+
.await
51+
.expect("success");
52+
53+
let log_contents = logs_rx.contents();
54+
assert!(log_contents.contains("token bucket for RetryPartition { name: \"dontcare-us-west-2\" } added to config bag"));
55+
56+
""",
57+
*codegenScope,
58+
)
59+
}
60+
61+
tokioTest("user_config_retry_partition") {
62+
val moduleName = ctx.moduleUseName()
63+
rustTemplate(
64+
"""
65+
let (_logs, logs_rx) = #{capture_test_logs}();
66+
let (http_client, _rx) = #{capture_request}(#{None});
67+
let client_config = $moduleName::Config::builder()
68+
.http_client(http_client)
69+
.region(#{Region}::new("us-west-2"))
70+
.credentials_provider(#{Credentials}::for_tests())
71+
.retry_partition(#{RetryPartition}::new("user-partition"))
72+
.build();
73+
74+
let client = $moduleName::Client::from_conf(client_config);
75+
76+
let _ = client
77+
.some_operation()
78+
.send()
79+
.await
80+
.expect("success");
81+
82+
let log_contents = logs_rx.contents();
83+
assert!(log_contents.contains("token bucket for RetryPartition { name: \"user-partition\" } added to config bag"));
84+
85+
""",
86+
*codegenScope,
87+
"RetryPartition" to RuntimeType.smithyRuntime(ctx.runtimeConfig).resolve("client::retries::RetryPartition"),
88+
)
89+
}
90+
}
91+
}
92+
}
93+
}

codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/client/FluentClientDecorator.kt

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,10 @@ sealed class FluentClientSection(name: String) : Section(name) {
8989
/** Write custom code for adding additional client plugins to base_client_runtime_plugins */
9090
data class AdditionalBaseClientPlugins(val plugins: String, val config: String) :
9191
FluentClientSection("AdditionalBaseClientPlugins")
92+
93+
/** Write additional code before plugins are configured */
94+
data class BeforeBaseClientPluginSetup(val config: String) :
95+
FluentClientSection("BeforeBaseClientPluginSetup")
9296
}
9397

9498
abstract class FluentClientCustomization : NamedCustomization<FluentClientSection>()

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

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -266,11 +266,14 @@ private fun baseClientRuntimePluginsFn(
266266
::std::mem::swap(&mut config.runtime_plugins, &mut configured_plugins);
267267
#{update_bmv}
268268
269+
let default_retry_partition = ${codegenContext.serviceShape.sdkId().dq()};
270+
#{before_plugin_setup}
271+
269272
let mut plugins = #{RuntimePlugins}::new()
270273
// defaults
271274
.with_client_plugins(#{default_plugins}(
272275
#{DefaultPluginParams}::new()
273-
.with_retry_partition_name(${codegenContext.serviceShape.sdkId().dq()})
276+
.with_retry_partition_name(default_retry_partition)
274277
.with_behavior_version(config.behavior_version.expect(${behaviorVersionError.dq()}))
275278
))
276279
// user config
@@ -299,6 +302,13 @@ private fun baseClientRuntimePluginsFn(
299302
FluentClientSection.AdditionalBaseClientPlugins("plugins", "config"),
300303
)
301304
},
305+
"before_plugin_setup" to
306+
writable {
307+
writeCustomizations(
308+
customizations,
309+
FluentClientSection.BeforeBaseClientPluginSetup("config"),
310+
)
311+
},
302312
"DefaultPluginParams" to rt.resolve("client::defaults::DefaultPluginParams"),
303313
"default_plugins" to rt.resolve("client::defaults::default_plugins"),
304314
"NoAuthRuntimePlugin" to rt.resolve("client::auth::no_auth::NoAuthRuntimePlugin"),

codegen-client/src/test/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/ConfigOverrideRuntimePluginGeneratorTest.kt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,7 @@ internal class ConfigOverrideRuntimePluginGeneratorTest {
182182
"ShouldAttempt" to
183183
RuntimeType.smithyRuntimeApi(runtimeConfig)
184184
.resolve("client::retries::ShouldAttempt"),
185+
"TokenBucket" to RuntimeType.smithyRuntime(runtimeConfig).resolve("client::retries::TokenBucket"),
185186
)
186187
rustCrate.testModule {
187188
unitTest("test_operation_overrides_retry_config") {
@@ -199,6 +200,7 @@ internal class ConfigOverrideRuntimePluginGeneratorTest {
199200
200201
let mut layer = #{Layer}::new("test");
201202
layer.store_put(#{RequestAttempts}::new(1));
203+
layer.store_put(#{TokenBucket}::default());
202204
203205
let mut cfg = #{ConfigBag}::of_layers(vec![layer]);
204206
let client_config_layer = client_config.config;

rust-runtime/Cargo.lock

Lines changed: 22 additions & 22 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)