Samrat002 commented on code in PR #28236:
URL: https://github.com/apache/flink/pull/28236#discussion_r3296120245
##########
flink-filesystems/flink-s3-fs-native/src/main/java/org/apache/flink/fs/s3native/S3ClientProvider.java:
##########
@@ -141,6 +148,13 @@ private S3ClientProvider(
this.checksumValidation = checksumValidation;
this.maxConnections = maxConnections;
this.maxRetries = maxRetries;
+ this.retryBaseDelay =
+ Preconditions.checkNotNull(retryBaseDelay, "retryBaseDelay
must not be null");
+ this.retryThrottleBaseDelay =
+ Preconditions.checkNotNull(
+ retryThrottleBaseDelay, "retryThrottleBaseDelay must
not be null");
+ this.retryMaxBackoff =
Review Comment:
Invalid values are possible. Add condition check for `negative durations`,
`zero durations`, `maxBackoff < baseDelay`, `maxBackoff < throttleBaseDelay`
##########
flink-filesystems/flink-s3-fs-native/src/main/java/org/apache/flink/fs/s3native/NativeS3FileSystemFactory.java:
##########
@@ -243,10 +243,35 @@ public class NativeS3FileSystemFactory implements
FileSystemFactory {
.intType()
.defaultValue(3)
.withDescription(
- "Maximum number of retry attempts for failed S3
requests. "
- + "Uses the AWS SDK's default retry
strategy (exponential backoff with jitter). "
+ "Maximum number of retries for failed S3 requests
(excluding the initial attempt). "
+ "Set to 0 to disable retries.");
+ public static final ConfigOption<Duration> RETRY_BASE_DELAY =
+ ConfigOptions.key("s3.retry.base-delay")
+ .durationType()
+ .defaultValue(Duration.ofMillis(100))
+ .withDescription(
+ "Base delay for exponential backoff on
non-throttle retries. "
+ + "Each retry waits a random duration
between 0 and "
+ + "min(base-delay * 2^attempt,
max-backoff).");
+
+ public static final ConfigOption<Duration> RETRY_THROTTLE_BASE_DELAY =
+ ConfigOptions.key("s3.retry.throttle.base-delay")
+ .durationType()
+ .defaultValue(Duration.ofSeconds(1))
+ .withDescription(
+ "Base delay for exponential backoff on throttle
retries (HTTP 429, 503). "
+ + "Each retry waits a random duration
between 0 and "
+ + "min(throttle-base-delay * 2^attempt,
max-backoff).");
Review Comment:
Use `Uses exponential backoff with jitter capped by max-backoff.` rather
than a mathematical expression.
In the future, when the logic evolves may lead to misleading.
##########
flink-filesystems/flink-s3-fs-native/src/test/java/org/apache/flink/fs/s3native/NativeS3FileSystemFactoryTest.java:
##########
@@ -178,6 +178,50 @@ void testMaxRetriesExplicitlyConfigured() throws Exception
{
assertThat(createFs(config).getClientProvider().getMaxRetries()).isEqualTo(5);
}
+ // --- Retry backoff ---
+
+ @Test
+ void testRetryBaseDelayDefault() throws Exception {
Review Comment:
No tests for actual retry strategy construction.
--
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]
