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 #4903

Merged
merged 8 commits into from
Feb 12, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a5ad71b
Enable setting multipart to true/false by defining it as a custom cli…
anirudh9391 Feb 8, 2024
01c5cb9
Refactor indentation
anirudh9391 Feb 8, 2024
b3bed43
Checking if multi-part is enabled before logging a debug message
anirudh9391 Feb 11, 2024
28c5d37
Updated Javadocs
anirudh9391 Feb 11, 2024
f843a5d
fix checkstyle
anirudh9391 Feb 11, 2024
ca25675
Modified logging message and added test case to enable cross region a…
anirudh9391 Feb 12, 2024
85e3a24
Fixed checkstyle
anirudh9391 Feb 12, 2024
9960c41
Fixed checkstyle issues
anirudh9391 Feb 12, 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
8 changes: 2 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,7 @@ private static Supplier<S3AsyncClient> defaultS3AsyncClient() {
if (crtInClasspath()) {
return S3AsyncClient::crtCreate;
}
return S3AsyncClient::create;
return () -> S3AsyncClient.builder().multipartEnabled(true).build();
anirudh9391 marked this conversation as resolved.
Show resolved Hide resolved
}

private static boolean crtInClasspath() {
Expand Down
64 changes: 47 additions & 17 deletions ...src/test/java/software/amazon/awssdk/transfer/s3/internal/TransferManagerLoggingTest.java
View file
Open in desktop
anirudh9391 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,9 @@

import static org.assertj.core.api.Assertions.assertThat;

import java.util.HashSet;
import java.util.List;
import java.util.Set;
import org.apache.logging.log4j.Level;
import org.apache.logging.log4j.core.LogEvent;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import software.amazon.awssdk.auth.credentials.AwsBasicCredentials;
import software.amazon.awssdk.regions.Region;
Expand All @@ -35,33 +32,66 @@ class TransferManagerLoggingTest {
@Test
void transferManager_withCrtClient_shouldNotLogWarnMessages() {

try (S3AsyncClient s3Crt = 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()) {
try (S3AsyncClient s3AsyncClient = S3AsyncClient.crtBuilder()
.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();
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 All @@ -72,4 +102,4 @@ private static void assertLogged(List<LogEvent> events, org.apache.logging.log4j
assertThat(msg).isEqualTo(message);
assertThat(event.getLevel()).isEqualTo(level);
}
}
}
6 changes: 2 additions & 4 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){};

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();
}
}
}
7 changes: 5 additions & 2 deletions 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":{
Copy link
Contributor

Choose a reason for hiding this comment

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

This also adds the multipart-enable config and method to the sync version of the client, as it adds the method the S3BaseClientBuilder interface which is used by both the sync and async builders:

S3Client client = S3Client.builder()
    .multipartEnabled(true) // <--- should not be available on sync builder
    .build();

I think we should avoid being able to define this config on the sync client and keep it on the async client only

"documentation":"Enables automatic conversion of put and copy method to their equivalent multipart operation.",
"type":"boolean"
}
},
"endpointAuthSchemeConfig": {
Expand All @@ -285,4 +288,4 @@
},
"useSraAuth": true,
"enableGenerateCompiledEndpointRules": true
}
}
Loading