gianm commented on code in PR #19892:
URL: https://github.com/apache/druid/pull/19892#discussion_r3725942155
##########
docs/development/extensions-core/s3.md:
##########
@@ -142,6 +144,61 @@ For example, to set the region to 'us-east-1' through
system properties:
|`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
+
+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
+Druid when that flag's default flips. For `standard` and `adaptive`, Druid
also opts in to the behavior AWS
+documents for those modes, rather than quietly falling back to any legacy
behavior the SDK may have.
+
+|Mode|Behavior|
+|----|--------|
+|`standard`|Error classification consistent with the other AWS SDK
implementations, and the mode AWS recommends for all workloads.|
+|`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
Review Comment:
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.
##########
cloud/aws-common/src/main/java/org/apache/druid/common/aws/AWSClientConfig.java:
##########
@@ -36,6 +44,68 @@ public class AWSClientConfig
/** AWS SDK v2's own default. */
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
Review Comment:
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.
##########
docs/development/extensions-core/s3.md:
##########
@@ -142,6 +144,61 @@ For example, to set the region to 'us-east-1' through
system properties:
|`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
+
+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
+Druid when that flag's default flips. For `standard` and `adaptive`, Druid
also opts in to the behavior AWS
+documents for those modes, rather than quietly falling back to any legacy
behavior the SDK may have.
+
+|Mode|Behavior|
+|----|--------|
+|`standard`|Error classification consistent with the other AWS SDK
implementations, and the mode AWS recommends for all workloads.|
+|`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
+
+AWS recommends `standard` as the default and `adaptive` only for workloads
that are *single-resource,
+throttling-heavy, and latency-tolerant*. S3 applies its request-rate limits
per key prefix, so "single-resource"
+means a process that concentrates its requests on one prefix.
+
+That maps onto Druid roughly as follows. Because a peon runs one task, you can
select a mode per task type — through
+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|
+|-------|--------------|---------|
+|MSQ and compaction peons|`adaptive`|Segment output goes to one new version
prefix and shuffle output to one prefix per query, which is exactly the
concentration that provokes throttling. Batch work tolerates the added latency.|
+|Batch ingestion peons|`standard`, or `adaptive` if throttled|Same shape as
above at lower request rates.|
+|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
+
+Every mode carries a retry quota: a token bucket, held per client instance and
never shared across processes, that
+stops retries once it is exhausted so the client fails fast instead of adding
load a struggling service cannot
+absorb. It only engages under sustained failure — roughly a 32% failure rate
for throttling errors — and is inert
+otherwise. Choosing between the modes does not change whether it is present.
+
+S3 reports throttling as `SlowDown` and `503`, which the SDK classifies as
throttling rather than transient errors.
+Those get a longer base backoff than transient failures, and they are what
`adaptive`'s rate limiter reacts to.
+
+### Retries are layered
+
+`druid.s3.maxRetryAttempts` applies per HTTP request. Druid retries again on
top of it, and the two multiply:
+
+|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`|
Review Comment:
Operator docs shouldn't refer to Java classes/functions such as
`S3Utils.retryS3Operation`.
##########
docs/development/extensions-core/s3.md:
##########
@@ -142,6 +144,61 @@ For example, to set the region to 'us-east-1' through
system properties:
|`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
+
+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
+Druid when that flag's default flips. For `standard` and `adaptive`, Druid
also opts in to the behavior AWS
+documents for those modes, rather than quietly falling back to any legacy
behavior the SDK may have.
+
+|Mode|Behavior|
+|----|--------|
+|`standard`|Error classification consistent with the other AWS SDK
implementations, and the mode AWS recommends for all workloads.|
+|`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
+
+AWS recommends `standard` as the default and `adaptive` only for workloads
that are *single-resource,
+throttling-heavy, and latency-tolerant*. S3 applies its request-rate limits
per key prefix, so "single-resource"
+means a process that concentrates its requests on one prefix.
+
+That maps onto Druid roughly as follows. Because a peon runs one task, you can
select a mode per task type — through
+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|
+|-------|--------------|---------|
+|MSQ and compaction peons|`adaptive`|Segment output goes to one new version
prefix and shuffle output to one prefix per query, which is exactly the
concentration that provokes throttling. Batch work tolerates the added latency.|
+|Batch ingestion peons|`standard`, or `adaptive` if throttled|Same shape as
above at lower request rates.|
+|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
Review Comment:
Maybe better to link to
https://docs.aws.amazon.com/sdkref/latest/guide/feature-retry-behavior.html vs
trying to summarize it.
##########
docs/development/extensions-core/s3.md:
##########
@@ -142,6 +144,61 @@ For example, to set the region to 'us-east-1' through
system properties:
|`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
Review Comment:
Linking to
https://docs.aws.amazon.com/sdkref/latest/guide/feature-retry-behavior.html
would be useful in this section.
##########
cloud/aws-common/src/main/java/org/apache/druid/common/aws/AWSClientConfig.java:
##########
@@ -80,6 +150,30 @@ public class AWSClientConfig
@Nullable
private Integer maxConnections = null;
+ /**
+ * Retry strategy applied to every AWS client built from this config.
+ * <p>
+ * Setting this at all is deliberate: left unset, the SDK picks its own
default, and which one it picks depends on
+ * the {@code aws.newRetries2026} migration flag. Naming the mode here keeps
retry behavior stable across SDK
+ * upgrades instead of changing under Druid when that flag's default flips.
+ */
+ @JsonProperty
+ private RetryMode retryMode = RetryMode.STANDARD;
+
+ /**
+ * Total attempts per request, including the first. Maps directly to the
SDK's {@code maxAttempts}, so 1 disables
Review Comment:
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).
##########
docs/development/extensions-core/s3.md:
##########
@@ -142,6 +144,61 @@ For example, to set the region to 'us-east-1' through
system properties:
|`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
+
+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
Review Comment:
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.
##########
docs/development/extensions-core/s3.md:
##########
@@ -142,6 +144,61 @@ For example, to set the region to 'us-east-1' through
system properties:
|`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
+
+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
+Druid when that flag's default flips. For `standard` and `adaptive`, Druid
also opts in to the behavior AWS
+documents for those modes, rather than quietly falling back to any legacy
behavior the SDK may have.
+
+|Mode|Behavior|
+|----|--------|
+|`standard`|Error classification consistent with the other AWS SDK
implementations, and the mode AWS recommends for all workloads.|
+|`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
+
+AWS recommends `standard` as the default and `adaptive` only for workloads
that are *single-resource,
+throttling-heavy, and latency-tolerant*. S3 applies its request-rate limits
per key prefix, so "single-resource"
+means a process that concentrates its requests on one prefix.
+
+That maps onto Druid roughly as follows. Because a peon runs one task, you can
select a mode per task type — through
+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|
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
