From 442a13c56697267357449ffa77c70e5520555aba Mon Sep 17 00:00:00 2001 From: John Viegas Date: Wed, 5 Apr 2023 19:14:04 +0530 Subject: [PATCH 1/5] Expose StandardRetryOptions in the S3CrtClient Interface --- .../s3/internal/crt/S3CrtAsyncHttpClient.java | 3 +++ .../internal/crt/S3NativeClientConfiguration.java | 13 +++++++++++++ .../s3/internal/crt/S3CrtAsyncHttpClientTest.java | 6 ++++++ 3 files changed, 22 insertions(+) diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtAsyncHttpClient.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtAsyncHttpClient.java index 13c708acf43e..9681c8c14f94 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtAsyncHttpClient.java +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtAsyncHttpClient.java @@ -78,6 +78,9 @@ private S3CrtAsyncHttpClient(Builder builder) { .withInitialReadWindowSize(initialWindowSize) .withReadBackpressureEnabled(true); + if (s3NativeClientConfiguration.standardRetryOptions() != null) { + this.s3ClientOptions.withStandardRetryOptions(s3NativeClientConfiguration.standardRetryOptions()); + } Optional.ofNullable(s3NativeClientConfiguration.proxyOptions()).ifPresent(s3ClientOptions::withProxyOptions); Optional.ofNullable(s3NativeClientConfiguration.connectionTimeout()) .map(Duration::toMillis) diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/crt/S3NativeClientConfiguration.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/crt/S3NativeClientConfiguration.java index 0fa9b235ebe7..e9ed2be52aab 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/crt/S3NativeClientConfiguration.java +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/crt/S3NativeClientConfiguration.java @@ -27,6 +27,7 @@ import software.amazon.awssdk.crt.http.HttpMonitoringOptions; import software.amazon.awssdk.crt.http.HttpProxyOptions; import software.amazon.awssdk.crt.io.ClientBootstrap; +import software.amazon.awssdk.crt.io.StandardRetryOptions; import software.amazon.awssdk.crt.io.TlsCipherPreference; import software.amazon.awssdk.crt.io.TlsContext; import software.amazon.awssdk.crt.io.TlsContextOptions; @@ -43,6 +44,7 @@ public class S3NativeClientConfiguration implements SdkAutoCloseable { private static final long DEFAULT_TARGET_THROUGHPUT_IN_GBPS = 10; private final String signingRegion; + private final StandardRetryOptions standardRetryOptions; private final ClientBootstrap clientBootstrap; private final CrtCredentialsProviderAdapter credentialProviderAdapter; private final CredentialsProvider credentialsProvider; @@ -97,6 +99,7 @@ public S3NativeClientConfiguration(Builder builder) { this.connectionTimeout = null; this.httpMonitoringOptions = null; } + this.standardRetryOptions = builder.standardRetryOptions; } public HttpMonitoringOptions httpMonitoringOptions() { @@ -140,6 +143,10 @@ public int maxConcurrency() { return maxConcurrency; } + public StandardRetryOptions standardRetryOptions() { + return standardRetryOptions; + } + public URI endpointOverride() { return endpointOverride; } @@ -169,6 +176,7 @@ public static final class Builder { private URI endpointOverride; private Boolean checksumValidationEnabled; private S3CrtHttpConfiguration httpConfiguration; + private StandardRetryOptions standardRetryOptions; private Builder() { } @@ -224,5 +232,10 @@ public Builder httpConfiguration(S3CrtHttpConfiguration httpConfiguration) { this.httpConfiguration = httpConfiguration; return this; } + + public Builder standardRetryOptions(StandardRetryOptions standardRetryOptions) { + this.standardRetryOptions = standardRetryOptions; + return this; + } } } diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtAsyncHttpClientTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtAsyncHttpClientTest.java index 16a302bf7f8d..f1eb68e1693f 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtAsyncHttpClientTest.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtAsyncHttpClientTest.java @@ -37,6 +37,8 @@ import org.mockito.Mockito; import software.amazon.awssdk.core.interceptor.trait.HttpChecksum; import software.amazon.awssdk.crt.http.HttpRequest; +import software.amazon.awssdk.crt.io.ExponentialBackoffRetryOptions; +import software.amazon.awssdk.crt.io.StandardRetryOptions; import software.amazon.awssdk.crt.s3.ChecksumAlgorithm; import software.amazon.awssdk.crt.s3.S3Client; import software.amazon.awssdk.crt.s3.S3ClientOptions; @@ -314,6 +316,9 @@ void build_shouldPassThroughParameters() { S3NativeClientConfiguration.builder() .maxConcurrency(100) .signingRegion("us-west-2") + .standardRetryOptions( + new StandardRetryOptions() + .withBackoffRetryOptions(new ExponentialBackoffRetryOptions().withMaxRetries(7))) .httpConfiguration(S3CrtHttpConfiguration.builder() .connectionTimeout(Duration.ofSeconds(1)) .connectionHealthConfiguration(c -> c.minimumThroughputInBps(1024L) @@ -325,6 +330,7 @@ void build_shouldPassThroughParameters() { (S3CrtAsyncHttpClient) S3CrtAsyncHttpClient.builder().s3ClientConfiguration(configuration).build(); S3ClientOptions clientOptions = client.s3ClientOptions(); assertThat(clientOptions.getConnectTimeoutMs()).isEqualTo(1000); + assertThat(clientOptions.getStandardRetryOptions().getBackoffRetryOptions().getMaxRetries()).isEqualTo(7); assertThat(clientOptions.getMaxConnections()).isEqualTo(100); assertThat(clientOptions.getMonitoringOptions()).satisfies(options -> { assertThat(options.getMinThroughputBytesPerSecond()).isEqualTo(1024); From 602fd9a29e6ca2c8ddc1a7affdd657774f5ce003 Mon Sep 17 00:00:00 2001 From: John Viegas Date: Wed, 5 Apr 2023 21:26:58 +0530 Subject: [PATCH 2/5] Added wrapper class for CRT retry config --- .../services/s3/S3CrtAsyncClientBuilder.java | 28 +++++ .../s3/crt/S3CrtRetryConfiguration.java | 112 ++++++++++++++++++ .../internal/crt/DefaultS3CrtAsyncClient.java | 27 ++++- 3 files changed, 162 insertions(+), 5 deletions(-) create mode 100644 services/s3/src/main/java/software/amazon/awssdk/services/s3/crt/S3CrtRetryConfiguration.java diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/S3CrtAsyncClientBuilder.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/S3CrtAsyncClientBuilder.java index 5d83e87bee13..5e16c9f9598f 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/S3CrtAsyncClientBuilder.java +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/S3CrtAsyncClientBuilder.java @@ -22,6 +22,7 @@ import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider; import software.amazon.awssdk.regions.Region; import software.amazon.awssdk.services.s3.crt.S3CrtHttpConfiguration; +import software.amazon.awssdk.services.s3.crt.S3CrtRetryConfiguration; import software.amazon.awssdk.services.s3.model.GetObjectRequest; import software.amazon.awssdk.services.s3.model.PutObjectRequest; import software.amazon.awssdk.utils.Validate; @@ -161,6 +162,14 @@ public interface S3CrtAsyncClientBuilder extends SdkBuilder retryConfigurationBuilder) { + Validate.paramNotNull(retryConfigurationBuilder, "configurationBuilder"); + return retryConfiguration(S3CrtRetryConfiguration.builder() + .applyMutation(retryConfigurationBuilder) + .build()); + } + + + + + @Override S3AsyncClient build(); } \ No newline at end of file diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/crt/S3CrtRetryConfiguration.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/crt/S3CrtRetryConfiguration.java new file mode 100644 index 000000000000..a5aaf27c36b9 --- /dev/null +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/crt/S3CrtRetryConfiguration.java @@ -0,0 +1,112 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package software.amazon.awssdk.services.s3.crt; + +import java.util.Objects; +import software.amazon.awssdk.annotations.Immutable; +import software.amazon.awssdk.annotations.SdkPublicApi; +import software.amazon.awssdk.annotations.ThreadSafe; +import software.amazon.awssdk.services.s3.S3CrtAsyncClientBuilder; +import software.amazon.awssdk.utils.Validate; +import software.amazon.awssdk.utils.builder.CopyableBuilder; +import software.amazon.awssdk.utils.builder.ToCopyableBuilder; + +/** + * Retry option configuration for AWS CRT-based S3 client. + * + * @see S3CrtAsyncClientBuilder#retryConfiguration + */ +@SdkPublicApi +@Immutable +@ThreadSafe +public final class S3CrtRetryConfiguration implements ToCopyableBuilder { + private final Integer numRetries; + + private S3CrtRetryConfiguration(DefaultBuilder builder) { + Validate.isPositiveOrNull(builder.numRetries, "numRetries"); + this.numRetries = builder.numRetries; + } + + /** + * Creates a default builder for {@link S3CrtRetryConfiguration}. + */ + public static Builder builder() { + return new S3CrtRetryConfiguration.DefaultBuilder(); + } + + /** + * Return the amount of time to wait when initially establishing a connection before giving up and timing out. + */ + public Integer numRetries() { + return numRetries; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + S3CrtRetryConfiguration that = (S3CrtRetryConfiguration) o; + return Objects.equals(numRetries, that.numRetries); + } + + @Override + public int hashCode() { + return numRetries != null ? numRetries.hashCode() : 0; + } + + @Override + public Builder toBuilder() { + return new S3CrtRetryConfiguration.DefaultBuilder(this); + } + + public interface Builder extends CopyableBuilder { + + /** + * Configure the maximum number of times that a single request should be retried. + * @param numRetries + * @return The builder of the method chaining. + */ + Builder numRetries(Integer numRetries); + + } + + private static final class DefaultBuilder implements Builder { + private Integer numRetries; + + private DefaultBuilder() { + } + + private DefaultBuilder(S3CrtRetryConfiguration crtRetryConfiguration) { + this.numRetries = crtRetryConfiguration.numRetries; + } + + @Override + public Builder numRetries(Integer numRetries) { + this.numRetries = numRetries; + return this; + } + + @Override + public S3CrtRetryConfiguration build() { + return new S3CrtRetryConfiguration(this); + } + } +} diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/crt/DefaultS3CrtAsyncClient.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/crt/DefaultS3CrtAsyncClient.java index d8c690f272cd..ffa7c9bf9213 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/crt/DefaultS3CrtAsyncClient.java +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/crt/DefaultS3CrtAsyncClient.java @@ -37,6 +37,8 @@ import software.amazon.awssdk.core.internal.util.ClassLoaderHelper; import software.amazon.awssdk.core.retry.RetryPolicy; import software.amazon.awssdk.core.signer.NoOpSigner; +import software.amazon.awssdk.crt.io.ExponentialBackoffRetryOptions; +import software.amazon.awssdk.crt.io.StandardRetryOptions; import software.amazon.awssdk.http.SdkHttpExecutionAttributes; import software.amazon.awssdk.regions.Region; import software.amazon.awssdk.services.s3.DelegatingS3AsyncClient; @@ -44,6 +46,7 @@ import software.amazon.awssdk.services.s3.S3Configuration; import software.amazon.awssdk.services.s3.S3CrtAsyncClientBuilder; import software.amazon.awssdk.services.s3.crt.S3CrtHttpConfiguration; +import software.amazon.awssdk.services.s3.crt.S3CrtRetryConfiguration; import software.amazon.awssdk.services.s3.model.CopyObjectRequest; import software.amazon.awssdk.services.s3.model.CopyObjectResponse; import software.amazon.awssdk.services.s3.model.GetObjectRequest; @@ -96,21 +99,28 @@ private static S3CrtAsyncHttpClient.Builder initializeS3CrtAsyncHttpClient(Defau Validate.isPositiveOrNull(builder.targetThroughputInGbps, "targetThroughputInGbps"); Validate.isPositiveOrNull(builder.minimalPartSizeInBytes, "minimalPartSizeInBytes"); - S3NativeClientConfiguration s3NativeClientConfiguration = + S3NativeClientConfiguration.Builder nativeClientBuilder = S3NativeClientConfiguration.builder() .checksumValidationEnabled(builder.checksumValidationEnabled) .targetThroughputInGbps(builder.targetThroughputInGbps) .partSizeInBytes(builder.minimalPartSizeInBytes) .maxConcurrency(builder.maxConcurrency) - .signingRegion(builder.region == null ? null : builder.region.id()) + .signingRegion(builder.region == null ? null + : + builder.region.id()) .endpointOverride(builder.endpointOverride) .credentialsProvider(builder.credentialsProvider) .readBufferSizeInBytes(builder.readBufferSizeInBytes) - .httpConfiguration(builder.httpConfiguration) - .build(); + .httpConfiguration(builder.httpConfiguration); + if (builder.retryConfiguration != null) { + nativeClientBuilder.standardRetryOptions( + new StandardRetryOptions() + .withBackoffRetryOptions(new ExponentialBackoffRetryOptions() + .withMaxRetries(builder.retryConfiguration.numRetries()))); + } return S3CrtAsyncHttpClient.builder() - .s3ClientConfiguration(s3NativeClientConfiguration); + .s3ClientConfiguration(nativeClientBuilder.build()); } public static final class DefaultS3CrtClientBuilder implements S3CrtAsyncClientBuilder { @@ -123,6 +133,7 @@ public static final class DefaultS3CrtClientBuilder implements S3CrtAsyncClientB private URI endpointOverride; private Boolean checksumValidationEnabled; private S3CrtHttpConfiguration httpConfiguration; + private S3CrtRetryConfiguration retryConfiguration; public AwsCredentialsProvider credentialsProvider() { return credentialsProvider; @@ -206,6 +217,12 @@ public S3CrtAsyncClientBuilder httpConfiguration(S3CrtHttpConfiguration configur return this; } + @Override + public S3CrtAsyncClientBuilder retryConfiguration(S3CrtRetryConfiguration retryConfiguration) { + this.retryConfiguration = retryConfiguration; + return this; + } + @Override public S3CrtAsyncClient build() { return new DefaultS3CrtAsyncClient(this); From a688479b85ef2316daed8cb89f8297b6654b40df Mon Sep 17 00:00:00 2001 From: John Viegas Date: Wed, 5 Apr 2023 21:51:47 +0530 Subject: [PATCH 3/5] Added Null checks and test cases --- .../services/s3/S3CrtAsyncClientBuilder.java | 2 +- .../s3/crt/S3CrtRetryConfiguration.java | 4 +- .../internal/crt/DefaultS3CrtAsyncClient.java | 4 +- .../s3/crt/S3CrtRetryConfigurationTest.java | 41 +++++++++++++++++++ 4 files changed, 45 insertions(+), 6 deletions(-) create mode 100644 services/s3/src/test/java/software/amazon/awssdk/services/s3/crt/S3CrtRetryConfigurationTest.java diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/S3CrtAsyncClientBuilder.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/S3CrtAsyncClientBuilder.java index 5e16c9f9598f..f1861553b6dd 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/S3CrtAsyncClientBuilder.java +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/S3CrtAsyncClientBuilder.java @@ -195,7 +195,7 @@ default S3CrtAsyncClientBuilder httpConfiguration(Consumer retryConfigurationBuilder) { - Validate.paramNotNull(retryConfigurationBuilder, "configurationBuilder"); + Validate.paramNotNull(retryConfigurationBuilder, "retryConfigurationBuilder"); return retryConfiguration(S3CrtRetryConfiguration.builder() .applyMutation(retryConfigurationBuilder) .build()); diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/crt/S3CrtRetryConfiguration.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/crt/S3CrtRetryConfiguration.java index a5aaf27c36b9..bc1afbb67904 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/crt/S3CrtRetryConfiguration.java +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/crt/S3CrtRetryConfiguration.java @@ -37,7 +37,7 @@ public final class S3CrtRetryConfiguration implements ToCopyableBuilderS3CrtRetryConfiguration.builder().build()) + .withMessage("numRetries"); + } + + + +} From 3f11f6457af23d9362c4539811fbc8a1c8759b08 Mon Sep 17 00:00:00 2001 From: John Viegas Date: Thu, 6 Apr 2023 21:50:25 +0530 Subject: [PATCH 4/5] Handled PR comment on javadoc --- .../awssdk/services/s3/crt/S3CrtRetryConfiguration.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/crt/S3CrtRetryConfiguration.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/crt/S3CrtRetryConfiguration.java index bc1afbb67904..fa2e21e0ace4 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/crt/S3CrtRetryConfiguration.java +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/crt/S3CrtRetryConfiguration.java @@ -80,8 +80,12 @@ public Builder toBuilder() { public interface Builder extends CopyableBuilder { /** - * Configure the maximum number of times that a single request should be retried. - * @param numRetries + * Sets the maximum number of retries for a single HTTP request. + *

For example, if an upload operation is split into 4 HTTP service requests ( One for initiate, Three for + * uploadPart and one for completeUpload), then numRetries specifies the maximum number of retries for each failed + * request, not for the entire uploadObject operation. + * + * @param numRetries The maximum number of retries for a single HTTP request. * @return The builder of the method chaining. */ Builder numRetries(Integer numRetries); From b6c1ed2351cd521678ba80ae8cc1ae197e8e6a23 Mon Sep 17 00:00:00 2001 From: John Viegas Date: Thu, 6 Apr 2023 23:44:51 +0530 Subject: [PATCH 5/5] Handled NIT comments --- .../awssdk/services/s3/crt/S3CrtRetryConfiguration.java | 2 +- .../awssdk/services/s3/crt/S3CrtRetryConfigurationTest.java | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/crt/S3CrtRetryConfiguration.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/crt/S3CrtRetryConfiguration.java index fa2e21e0ace4..6ea6a1508ed9 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/crt/S3CrtRetryConfiguration.java +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/crt/S3CrtRetryConfiguration.java @@ -81,7 +81,7 @@ public interface Builder extends CopyableBuilder For example, if an upload operation is split into 4 HTTP service requests ( One for initiate, Three for + *

For example, if an upload operation is split into 5 HTTP service requests ( One for initiate, Three for * uploadPart and one for completeUpload), then numRetries specifies the maximum number of retries for each failed * request, not for the entire uploadObject operation. * diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/crt/S3CrtRetryConfigurationTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/crt/S3CrtRetryConfigurationTest.java index 26c3b1f03571..0fd6b7f9ad95 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/crt/S3CrtRetryConfigurationTest.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/crt/S3CrtRetryConfigurationTest.java @@ -17,11 +17,9 @@ import nl.jqno.equalsverifier.EqualsVerifier; import org.junit.jupiter.api.Test; -import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; import static org.assertj.core.api.Assertions.assertThatNullPointerException; - -public class S3CrtRetryConfigurationTest { +class S3CrtRetryConfigurationTest { @Test void equalsHashcode() {