feat: Add operator access to configure AWS retry policy. Default to STANDARD, recommended by AWS by capistrant · Pull Request #19892 · apache/druid

capistrant · 2026-08-05T20:46:17Z

Description

AWS sdk has made changes to how s3 retries are handled by the client. There are multiple policy decisions available. https://docs.aws.amazon.com/sdkref/latest/guide/feature-retry-behavior.html can be referenced for more information. The biggest change in this PR is ensuring that we are operating with STANDARD which is the general recommendation for policy going forward. ADAPTIVE is the special case policy that I want to expose to operators. We do update docs to suggest that ingestion tasks may be good fits for adaptive but we do not go as far as changing the policy for anything to use adaptive by default. If we find that it is far and away better, perhaps future releases will create clients with policies tailored to the job of the service.

Edit: Previously the PR gave guidance on retry mode selection per druid service type. On advice of review, we have scaled the docs way back to simply expose the new runtime property and link to the AWS docs on choosing a retry mode. I was overselling confidence in what mode is good for what service. If, in the future, we determine adaptive is better than standard for anything we will opt into that by default.

Release note

Explicitly change to the AWS Standard retry policy for the s3 client. The retry mode used by s3 client is configurable now in case operators want to customize. One such customization that may be useful is moving druid.s3.retryMode to adaptive for ingestion tasks. Operators should use caution when making changes the the retry mode, and consult https://docs.aws.amazon.com/sdkref/latest/guide/feature-retry-behavior.html before making any decisions for critical service configuration.

Key changed/added classes in this PR

AWSClientConfig
ServerSideEncryptingAmazonS3

This PR has:

been self-reviewed.
added documentation for new or modified features or behaviors.
a release note entry in the PR description.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added or updated version, license, or notice information in licenses.yaml
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
added integration tests.
been tested in a test Druid cluster.

…D, recommended by AWS

-    AWSClientConfig config = mapperWithRuntimeInfo(new FixedProcessorsRuntimeInfo(32))
-        .readValue("{}", AWSClientConfig.class);
-    Assertions.assertEquals(128, config.getMaxConnections());
+    Assertions.assertNull(bind(Map.of()).isForceGlobalBucketAccessEnabled());


-        .readValue("{}", AWSClientConfig.class);
-    Assertions.assertEquals(128, config.getMaxConnections());
+    Assertions.assertNull(bind(Map.of()).isForceGlobalBucketAccessEnabled());
+    Assertions.assertNull(bind(Map.of("crossRegionAccessEnabled", true)).isForceGlobalBucketAccessEnabled());


gianm · 2026-08-06T04:19:25Z

  private static final int DEFAULT_MAX_CONNECTIONS_FLOOR = 50;

+  /**
+   * Selects the retry behavior AWS documents for {@code standard} and {@code adaptive} rather than the pre-2026


I found this javadoc hard to understand until reading https://docs.aws.amazon.com/sdkref/latest/guide/feature-retry-behavior.html. It would be helpful to reword the javadoc, or link to https://docs.aws.amazon.com/sdkref/latest/guide/feature-retry-behavior.html, or ideally.

I think it would also be fine to get rid of this constant and instead inline true into the calls to standardRetryStrategy and adaptiveRetryStrategy. Sometimes less is more.

gianm · 2026-08-06T04:26:50Z

+  private RetryMode retryMode = RetryMode.STANDARD;
+
+  /**
+   * Total attempts per request, including the first. Maps directly to the SDK's {@code maxAttempts}, so 1 disables


If 1 disables retries then this config should be named maxAttempts, not maxRetryAttempts. (1 retry attempt means there were 2 attempts: an initial attempt and a retry attempt).

gianm · 2026-08-06T04:38:40Z

 |`druid.storage.sse.kms.keyId`|AWS KMS key ID. This is used only when `druid.storage.sse.type` is `kms` and can be empty to use the default key ID.|None|
 |`druid.storage.sse.custom.base64EncodedKey`|Base64-encoded key. Should be specified if `druid.storage.sse.type` is `custom`.|None|

+## Retry behavior


Linking to https://docs.aws.amazon.com/sdkref/latest/guide/feature-retry-behavior.html would be useful in this section.

gianm · 2026-08-06T04:39:35Z

+## Retry behavior
+
+Druid names the AWS SDK retry strategy explicitly rather than accepting the SDK's default. The SDK's own default
+depends on the `aws.newRetries2026` migration flag, so leaving it unset would let retry behavior change underneath


I don't think mentioning this flag or the SDK defaults here is useful, because neither of them matter (since we're explicitly creating policies such that the flag and SDK defaults are ignored). Bringing them up makes it seem like something the operator might need to care about.

gianm · 2026-08-06T04:42:24Z

+|`adaptive`|`standard` plus a client-side rate limiter that slows requests down when S3 reports throttling. Unlike `standard`, it can delay or block the **initial** request, not only retries. The limiter covers every request made by one client instance, so throttling on one key prefix also slows requests to prefixes that are not being throttled.|
+|`legacy`|The SDK's legacy behavior, retained so a deployment can revert without a rollback. AWS recommends moving off it.|
+
+### Choosing a mode


I don't think this section should be here. If we really think that these suggestions are good then they should be implemented as defaults in code.

gianm · 2026-08-06T04:52:57Z

+a Kubernetes pod template, or `druid.indexer.fork.property.druid.s3.retryMode` in a task's context when using the
+MiddleManager task runner.
+
+|Process|Suggested mode|Reasoning|


I'm not sure we can make such specific claims about how prefixes work with rate limiting. The AWS docs, to me, seem vague as to what exactly the rate-limitable prefix is. They don't appear to promise that it's any particular prefix of the key. The page at https://repost.aws/knowledge-center/s3-prefix-nested-folders-difference suggests that AWS partitions the prefixes adaptively in some way that is possibly opaque to the user.

Anyway, we won't need to worry about this if we remove the subsection as I am also suggesting.

gianm · 2026-08-06T04:54:17Z

+|Historicals|`standard`|One client loads segments for every datasource the process serves, so a rate limiter tripped by one prefix would slow loads for unrelated ones. Segment loads can also sit on the query path.|
+|Brokers, Coordinator, Overlord|`standard`|Low request volume, and delaying an initial request costs query latency for no benefit.|
+
+### Retry quota


Maybe better to link to https://docs.aws.amazon.com/sdkref/latest/guide/feature-retry-behavior.html vs trying to summarize it.

gianm · 2026-08-06T04:55:02Z

+|Layer|Scope of one attempt|Attempts|Backoff cap|
+|-----|--------------------|--------|-----------|
+|`druid.s3.maxRetryAttempts`|A single HTTP request, such as one `UploadPart`|Set by the retry mode|`~20s`|
+|`S3Utils.retryS3Operation`|A whole logical operation, such as re-uploading an entire segment|10|`60s`|


Operator docs shouldn't refer to Java classes/functions such as S3Utils.retryS3Operation.

FrankChen021

Severity	Findings
P0	0
P1	2
P2	0
P3	0
Total	2

Reviewed 7 of 7 changed files. Retry state is shared across two clients, and custom input clients can bypass the process retry configuration.

This is an automated review by Codex GPT-5.6-Sol

FrankChen021 · 2026-08-06T12:36:35Z

      S3Configuration s3Config = S3Configuration.builder()
                                                .chunkedEncodingEnabled(!awsClientConfig.isDisableChunkedEncoding())
                                                .build();
+      final ClientOverrideConfiguration retryOverrides =


[P1] Give each S3 client its own retry strategy

This override captures one stateful RetryStrategy and hands the same instance to both the synchronous and asynchronous clients. The SDK's standard strategy stores circuit-breaker quota on the strategy instance, while adaptive additionally stores its rate limiter there, using a global scope. Consequently, throttled TransferManager uploads can drain retry quota or delay synchronous reads and listings, contrary to the documented per-client isolation. Build separate override configurations by calling getRetryStrategy() independently for each client.

FrankChen021 · 2026-08-06T12:36:35Z

    if (awsEndpointConfig != null && awsEndpointConfig.getSigningRegion() != null) {
      stsBuilder.region(Region.of(awsEndpointConfig.getSigningRegion()));
    }
+    if (awsClientConfig != null) {


[P1] Preserve global retry settings for custom input clients

Here and in the S3 setup above, null leaves the SDK policy unchanged. For S3InputSource this argument is the nullable per-spec clientConfig, not the Guice-bound process configuration. An ingestion spec that supplies properties or assumeRoleArn but omits clientConfig therefore builds new S3 and STS clients that silently ignore druid.s3.retryMode and maxRetryAttempts, including the task-context configuration recommended by the new documentation. Fall back to the injected process AWSClientConfig when no per-source override is provided and cover this path in a test.

regarding S3InputSource... I actually don't want to be adding that to scope in this PR. the docs explicitly talking so confidently about this config for tasks was a mistake and has been reverted.

…like s3 input source get strategy when needed

Add operator access to configure AWS retry policy. Default to STANDAR…

43c5ade

…D, recommended by AWS

github-actions Bot added Area - Documentation Area - Dependencies labels Aug 5, 2026

capistrant added 3 commits August 5, 2026 16:33

add missing dep

4dcd46f

modify some test structure

6154897

remove robot affinity for british spelling

7c7b3b8

github-advanced-security AI found potential problems Aug 5, 2026

View reviewed changes

fix test

2c57bb1

gianm reviewed Aug 6, 2026

View reviewed changes

FrankChen021 reviewed Aug 6, 2026

View reviewed changes

capistrant added 5 commits August 6, 2026 09:30

cleanup

69b2b19

Merge branch 'master' into s3-client-retry-policy-configurability

982bdf8

make retry strategy instance per client + ensure clients from things …

a1b4286

…like s3 input source get strategy when needed

actually don't do the 2nd comment suggestion

b0b3472

get explicit on saying the retry strategy object shouldnt be shared

fb241a8

gianm approved these changes Aug 6, 2026

View reviewed changes

Navigation Menu

Uh oh!

feat: Add operator access to configure AWS retry policy. Default to STANDARD, recommended by AWS - #19892

feat: Add operator access to configure AWS retry policy. Default to STANDARD, recommended by AWS#19892
capistrant wants to merge 10 commits into
apache:masterfrom
capistrant:s3-client-retry-policy-configurability

capistrant commented Aug 5, 2026 •

edited

Loading

Uh oh!

gianm Aug 6, 2026

Uh oh!

gianm Aug 6, 2026

Uh oh!

gianm Aug 6, 2026

Uh oh!

gianm Aug 6, 2026

Uh oh!

gianm Aug 6, 2026

Uh oh!

gianm Aug 6, 2026

Uh oh!

gianm Aug 6, 2026

Uh oh!

gianm Aug 6, 2026

Uh oh!

FrankChen021 left a comment

Uh oh!

FrankChen021 Aug 6, 2026

Uh oh!

FrankChen021 Aug 6, 2026

Uh oh!

capistrant Aug 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

capistrant commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Release note

Key changed/added classes in this PR

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

capistrant commented Aug 5, 2026 •

edited

Loading