FrankChen021 commented on code in PR #19892:
URL: https://github.com/apache/druid/pull/19892#discussion_r3728797989
##########
extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/ServerSideEncryptingAmazonS3.java:
##########
@@ -453,6 +462,11 @@ public static AwsCredentialsProvider
createAssumeRoleCredentialsProvider(
if (awsEndpointConfig != null && awsEndpointConfig.getSigningRegion() !=
null) {
stsBuilder.region(Region.of(awsEndpointConfig.getSigningRegion()));
}
+ if (awsClientConfig != null) {
Review Comment:
[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.
##########
extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/ServerSideEncryptingAmazonS3.java:
##########
@@ -373,10 +374,16 @@ public static ServerSideEncryptingAmazonS3.Builder
builder(
S3Configuration s3Config = S3Configuration.builder()
.chunkedEncodingEnabled(!awsClientConfig.isDisableChunkedEncoding())
.build();
+ final ClientOverrideConfiguration retryOverrides =
Review Comment:
[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.
--
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]
