Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable setting multipart to true/false by defining it as a custom client context parameter #4899

Closed
wants to merge 5 commits into from
Closed

Enable setting multipart to true/false by defining it as a custom client context parameter #4899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ad1f204
Enable setting multipart to true/false by defining it as a custom cli…
anirudh9391 Feb 8, 2024
3a8554f
Merge branch 'master' into anirudkr-multipart
anirudh9391 Feb 8, 2024
4622306
Enable setting multipart to true/false by defining it as a custom cli…
anirudh9391 Feb 8, 2024
028e728
Added one more test
anirudh9391 Feb 8, 2024
db06277
Merge branch 'master' into anirudkr-multipart
anirudh9391 Feb 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 0 additions & 13 deletions ...en/src/main/java/software/amazon/awssdk/codegen/poet/builder/AsyncClientBuilderClass.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,6 @@ public TypeSpec poetSpec() {

MultipartCustomization multipartCustomization = model.getCustomizationConfig().getMultipartCustomization();
if (multipartCustomization != null) {
builder.addMethod(multipartEnabledMethod(multipartCustomization));
builder.addMethod(multipartConfigMethods(multipartCustomization));
}

Expand Down Expand Up @@ -159,18 +158,6 @@ private MethodSpec tokenProviderMethod() {
.build();
}

private MethodSpec multipartEnabledMethod(MultipartCustomization multipartCustomization) {
return MethodSpec.methodBuilder("multipartEnabled")
.addAnnotation(Override.class)
.addModifiers(Modifier.PUBLIC)
.returns(builderInterfaceName)
.addParameter(Boolean.class, "enabled")
.addStatement("clientContextParams.put($N, enabled)",
multipartCustomization.getContextParamEnabledKey())
.addStatement("return this")
.build();
}

private MethodSpec multipartConfigMethods(MultipartCustomization multipartCustomization) {
ClassName mulitpartConfigClassName =
PoetUtils.classNameFromFqcn(multipartCustomization.getMultipartConfigurationClass());
Expand Down
10 changes: 4 additions & 6 deletions ...ger/src/main/java/software/amazon/awssdk/transfer/s3/internal/TransferManagerFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,7 @@ public static S3TransferManager createTransferManager(DefaultBuilder tmBuilder)
return new CrtS3TransferManager(transferConfiguration, s3AsyncClient, isDefaultS3AsyncClient);
}

if (s3AsyncClient.getClass().getName().equals("software.amazon.awssdk.services.s3.DefaultS3AsyncClient")) {
log.warn(() -> "The provided DefaultS3AsyncClient is not an instance of S3CrtAsyncClient, and thus multipart"
+ " upload/download feature is not enabled and resumable file upload is not supported. To benefit "
+ "from maximum throughput, consider using S3AsyncClient.crtBuilder().build() instead.");
} else {
if (!Boolean.TRUE.equals(s3AsyncClient.serviceClientConfiguration().multipartEnabled())) {
log.debug(() -> "The provided S3AsyncClient is not an instance of S3CrtAsyncClient, and thus multipart"
+ " upload/download feature may not be enabled and resumable file upload may not be supported.");
}
Expand All @@ -68,7 +64,9 @@ private static Supplier<S3AsyncClient> defaultS3AsyncClient() {
if (crtInClasspath()) {
return S3AsyncClient::crtCreate;
}
return S3AsyncClient::create;
return () -> S3AsyncClient.builder()
.multipartEnabled(true)
.build();
}

private static boolean crtInClasspath() {
Expand Down
55 changes: 45 additions & 10 deletions ...src/test/java/software/amazon/awssdk/transfer/s3/internal/TransferManagerLoggingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
import org.apache.logging.log4j.core.LogEvent;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.Mock;
import org.mockito.Mockito;
import software.amazon.awssdk.auth.credentials.AwsBasicCredentials;
import software.amazon.awssdk.regions.Region;
import software.amazon.awssdk.services.s3.S3AsyncClient;
Expand All @@ -35,33 +37,66 @@ class TransferManagerLoggingTest {
@Test
void transferManager_withCrtClient_shouldNotLogWarnMessages() {

try (S3AsyncClient s3Crt = S3AsyncClient.crtBuilder()
try (S3AsyncClient s3AsyncClient = S3AsyncClient.crtBuilder()
.region(Region.US_WEST_2)
.credentialsProvider(() -> AwsBasicCredentials.create("foo", "bar"))
.build();
LogCaptor logCaptor = LogCaptor.create(Level.WARN);
S3TransferManager tm = S3TransferManager.builder().s3Client(s3Crt).build()) {
LogCaptor logCaptor = LogCaptor.create(Level.DEBUG);
S3TransferManager tm = S3TransferManager.builder().s3Client(s3AsyncClient).build()) {
List<LogEvent> events = logCaptor.loggedEvents();
assertThat(events).isEmpty();
}
}

@Test
void transferManager_withJavaClient_shouldLogWarnMessage() {
void transferManager_withCustomJavaClientWhenMultiPartEnabledIsNull_shouldLogDebugMessage() {
try (S3AsyncClient s3AsyncClient = S3AsyncClient.builder()
.region(Region.US_WEST_2)
.credentialsProvider(() -> AwsBasicCredentials.create("foo", "bar"))
.build();
LogCaptor logCaptor = LogCaptor.create(Level.DEBUG);
S3TransferManager tm = S3TransferManager.builder().s3Client(s3AsyncClient).build()) {
List<LogEvent> events = logCaptor.loggedEvents();
assertLogged(events, Level.DEBUG, "The provided S3AsyncClient is not an instance of S3CrtAsyncClient, and thus multipart"
+ " upload/download feature may not be enabled and resumable file upload may not be supported.");
}
}

@Test
void transferManager_withCustomJavaClientWhenMultiPartEnabledIsFalse_shouldLogDebugMessage() {
try (S3AsyncClient s3AsyncClient = S3AsyncClient.builder()
.region(Region.US_WEST_2)
.credentialsProvider(() -> AwsBasicCredentials.create("foo", "bar"))
.multipartEnabled(false)
.build();
LogCaptor logCaptor = LogCaptor.create(Level.DEBUG);
S3TransferManager tm = S3TransferManager.builder().s3Client(s3AsyncClient).build()) {
List<LogEvent> events = logCaptor.loggedEvents();
assertLogged(events, Level.DEBUG, "The provided S3AsyncClient is not an instance of S3CrtAsyncClient, and thus multipart"
+ " upload/download feature may not be enabled and resumable file upload may not be supported.");
}
}

@Test
void transferManager_withDefaultJavaClient_shouldNotLogDebugMessage() {
try (LogCaptor logCaptor = LogCaptor.create(Level.DEBUG);
S3TransferManager tm = S3TransferManager.builder().build()) {
List<LogEvent> events = logCaptor.loggedEvents();
assertThat(events).isEmpty();
}
}

@Test
void transferManager_withCustomJavaClientMultiPartEnabled_shouldNotLogDebugMessage() {
try (S3AsyncClient s3Crt = S3AsyncClient.builder()
.region(Region.US_WEST_2)
.credentialsProvider(() -> AwsBasicCredentials.create("foo", "bar"))
.multipartEnabled(true)
.build();
LogCaptor logCaptor = LogCaptor.create(Level.WARN);
LogCaptor logCaptor = LogCaptor.create(Level.DEBUG);
S3TransferManager tm = S3TransferManager.builder().s3Client(s3Crt).build()) {
List<LogEvent> events = logCaptor.loggedEvents();
assertLogged(events, Level.WARN, "The provided DefaultS3AsyncClient is not an instance of S3CrtAsyncClient, and "
+ "thus multipart upload/download feature is not enabled and resumable file upload"
+ " is "
+ "not supported. To benefit from maximum throughput, consider using "
+ "S3AsyncClient.crtBuilder().build() instead.");
assertThat(events).isEmpty();
}
}

Expand Down
4 changes: 1 addition & 3 deletions .../main/java/software/amazon/awssdk/services/s3/internal/client/S3AsyncClientDecorator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@
public class S3AsyncClientDecorator {
public static final AttributeMap.Key<MultipartConfiguration> MULTIPART_CONFIGURATION_KEY =
new AttributeMap.Key<MultipartConfiguration>(MultipartConfiguration.class){};
public static final AttributeMap.Key<Boolean> MULTIPART_ENABLED_KEY =
new AttributeMap.Key<Boolean>(Boolean.class){};
Comment on lines -36 to -37
Copy link
Contributor

@L-Applin L-Applin Feb 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we dont need it anymore, we should also remove it from customization.config: "contextParamEnabledKey": "S3AsyncClientDecorator.MULTIPART_ENABLED_KEY" ln245

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay. I missed that one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it


public S3AsyncClientDecorator() {
}
Expand Down Expand Up @@ -62,7 +60,7 @@ private Predicate<S3AsyncClient> isCrossRegionEnabledAsync(AttributeMap clientCo
}

private Predicate<S3AsyncClient> isMultipartEnable(AttributeMap clientContextParams) {
Boolean multipartEnabled = clientContextParams.get(MULTIPART_ENABLED_KEY);
Boolean multipartEnabled = clientContextParams.get(S3ClientContextParams.MULTIPART_ENABLED);
return client -> multipartEnabled != null && multipartEnabled.booleanValue();
}
}
5 changes: 4 additions & 1 deletion services/s3/src/main/resources/codegen-resources/customization.config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,6 @@
"multipartConfigurationClass": "software.amazon.awssdk.services.s3.multipart.MultipartConfiguration",
"multipartConfigMethodDoc": "Configuration for multipart operation of this client.",
"multipartEnableMethodDoc": "Enables automatic conversion of put and copy method to their equivalent multipart operation.",
"contextParamEnabledKey": "S3AsyncClientDecorator.MULTIPART_ENABLED_KEY",
"contextParamConfigKey": "S3AsyncClientDecorator.MULTIPART_CONFIGURATION_KEY"
},
"interceptors": [
Expand Down Expand Up @@ -271,6 +270,10 @@
"CrossRegionAccessEnabled":{
"documentation":"Enables cross-region bucket access for this client",
"type":"boolean"
},
"multipartEnabled":{
"documentation":"Enables automatic conversion of put and copy method to their equivalent multipart operation.",
"type":"boolean"
}
},
"endpointAuthSchemeConfig": {
Expand Down