From 250dce3f34f9833593642254197d07d274442851 Mon Sep 17 00:00:00 2001 From: Olivier Lepage-Applin Date: Tue, 20 Feb 2024 10:47:41 -0500 Subject: [PATCH 1/5] Make Transfer Manager work by default with MultipartS3AsyncClient. --- .../s3/internal/GenericS3TransferManager.java | 18 +++-- .../s3/internal/TransferManagerFactory.java | 7 +- .../MultipartDownloadJavaBasedTest.java | 67 +++++++++++++++++++ 3 files changed, 86 insertions(+), 6 deletions(-) create mode 100644 services-custom/s3-transfer-manager/src/test/java/software/amazon/awssdk/transfer/s3/internal/MultipartDownloadJavaBasedTest.java diff --git a/services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/internal/GenericS3TransferManager.java b/services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/internal/GenericS3TransferManager.java index ec9f25a133c1..ecc62def60b6 100644 --- a/services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/internal/GenericS3TransferManager.java +++ b/services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/internal/GenericS3TransferManager.java @@ -33,6 +33,7 @@ import software.amazon.awssdk.core.exception.SdkClientException; import software.amazon.awssdk.core.exception.SdkException; import software.amazon.awssdk.core.internal.async.FileAsyncRequestBody; +import software.amazon.awssdk.services.s3.DelegatingS3AsyncClient; import software.amazon.awssdk.services.s3.S3AsyncClient; import software.amazon.awssdk.services.s3.internal.multipart.MultipartS3AsyncClient; import software.amazon.awssdk.services.s3.internal.resource.S3AccessPointResource; @@ -298,8 +299,7 @@ public Download download(DownloadRequest downloadReq try { assertNotUnsupportedArn(downloadRequest.getObjectRequest().bucket(), "download"); - CompletableFuture crtFuture = - s3AsyncClient.getObject(downloadRequest.getObjectRequest(), responseTransformer); + CompletableFuture crtFuture = doGetObject(downloadRequest.getObjectRequest(), responseTransformer); // Forward download cancellation to CRT future CompletableFutureUtils.forwardExceptionTo(returnFuture, crtFuture); @@ -341,9 +341,7 @@ private TransferProgressUpdater doDownloadFile( assertNotUnsupportedArn(downloadRequest.getObjectRequest().bucket(), "download"); - CompletableFuture crtFuture = - s3AsyncClient.getObject(downloadRequest.getObjectRequest(), - responseTransformer); + CompletableFuture crtFuture = doGetObject(downloadRequest.getObjectRequest(), responseTransformer); // Forward download cancellation to CRT future CompletableFutureUtils.forwardExceptionTo(returnFuture, crtFuture); @@ -511,4 +509,14 @@ private static boolean isMrapArn(Arn arn) { return !s3EndpointResource.region().isPresent(); } + + // todo remove once MultipartS3AsyncClient is complete + private CompletableFuture doGetObject( + GetObjectRequest getObjectRequest, AsyncResponseTransformer asyncResponseTransformer) { + S3AsyncClient clientToUse = s3AsyncClient; + if (s3AsyncClient instanceof MultipartS3AsyncClient) { + clientToUse = (S3AsyncClient) ((DelegatingS3AsyncClient) s3AsyncClient).delegate(); + } + return clientToUse.getObject(getObjectRequest, asyncResponseTransformer); + } } diff --git a/services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/internal/TransferManagerFactory.java b/services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/internal/TransferManagerFactory.java index a87d8d6fcd60..7b4041804a29 100644 --- a/services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/internal/TransferManagerFactory.java +++ b/services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/internal/TransferManagerFactory.java @@ -21,6 +21,7 @@ import software.amazon.awssdk.core.internal.util.ClassLoaderHelper; import software.amazon.awssdk.services.s3.S3AsyncClient; import software.amazon.awssdk.services.s3.internal.crt.S3CrtAsyncClient; +import software.amazon.awssdk.services.s3.internal.multipart.MultipartS3AsyncClient; import software.amazon.awssdk.transfer.s3.S3TransferManager; import software.amazon.awssdk.utils.Logger; @@ -56,6 +57,10 @@ public static S3TransferManager createTransferManager(DefaultBuilder tmBuilder) 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 (s3AsyncClient instanceof MultipartS3AsyncClient) { + log.warn(() -> "The provided S3AsyncClient is an instance of MultipartS3AsyncClient, and thus multipart" + + " download feature is not enabled. To benefit from all features, " + + "consider using S3AsyncClient.crtBuilder().build() instead."); } else { 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."); @@ -68,7 +73,7 @@ private static Supplier defaultS3AsyncClient() { if (crtInClasspath()) { return S3AsyncClient::crtCreate; } - return S3AsyncClient::create; + return S3AsyncClient.builder().multipartEnabled(true)::build; } private static boolean crtInClasspath() { diff --git a/services-custom/s3-transfer-manager/src/test/java/software/amazon/awssdk/transfer/s3/internal/MultipartDownloadJavaBasedTest.java b/services-custom/s3-transfer-manager/src/test/java/software/amazon/awssdk/transfer/s3/internal/MultipartDownloadJavaBasedTest.java new file mode 100644 index 000000000000..1b5c1063239f --- /dev/null +++ b/services-custom/s3-transfer-manager/src/test/java/software/amazon/awssdk/transfer/s3/internal/MultipartDownloadJavaBasedTest.java @@ -0,0 +1,67 @@ +/* + * 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.transfer.s3.internal; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.nio.file.Paths; +import java.util.concurrent.CompletableFuture; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import software.amazon.awssdk.core.async.AsyncResponseTransformer; +import software.amazon.awssdk.services.s3.S3AsyncClient; +import software.amazon.awssdk.services.s3.internal.multipart.MultipartS3AsyncClient; +import software.amazon.awssdk.services.s3.model.GetObjectRequest; +import software.amazon.awssdk.services.s3.model.GetObjectResponse; +import software.amazon.awssdk.services.s3.multipart.MultipartConfiguration; +import software.amazon.awssdk.transfer.s3.S3TransferManager; +import software.amazon.awssdk.transfer.s3.model.CompletedFileDownload; + +class MultipartDownloadJavaBasedTest { + private S3AsyncClient mockDelegate; + private MultipartS3AsyncClient s3Multi; + private S3TransferManager tm; + private UploadDirectoryHelper uploadDirectoryHelper; + private DownloadDirectoryHelper downloadDirectoryHelper; + private TransferManagerConfiguration configuration; + + @BeforeEach + public void methodSetup() { + mockDelegate = mock(S3AsyncClient.class); + s3Multi = MultipartS3AsyncClient.create(mockDelegate, MultipartConfiguration.builder().build()); + uploadDirectoryHelper = mock(UploadDirectoryHelper.class); + configuration = mock(TransferManagerConfiguration.class); + downloadDirectoryHelper = mock(DownloadDirectoryHelper.class); + tm = new GenericS3TransferManager(s3Multi, uploadDirectoryHelper, configuration, downloadDirectoryHelper); + } + + @Test + void usingMultipartDownload_shouldNotThrowException() { + GetObjectResponse response = GetObjectResponse.builder().build(); + when(mockDelegate.getObject(any(GetObjectRequest.class), any(AsyncResponseTransformer.class))) + .thenReturn(CompletableFuture.completedFuture(response)); + + CompletedFileDownload completedFileDownload = tm.downloadFile(d -> d.getObjectRequest(g -> g.bucket("bucket") + .key("key")) + .destination(Paths.get("."))) + .completionFuture() + .join(); + assertThat(completedFileDownload.response()).isEqualTo(response); + } +} From da1da9e9405cd0c9c197efad48928d8bf31e954b Mon Sep 17 00:00:00 2001 From: Olivier Lepage-Applin Date: Tue, 20 Feb 2024 11:40:13 -0500 Subject: [PATCH 2/5] changelog --- .changes/next-release/feature-AWSSDKforJavav2-787c575.json | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changes/next-release/feature-AWSSDKforJavav2-787c575.json diff --git a/.changes/next-release/feature-AWSSDKforJavav2-787c575.json b/.changes/next-release/feature-AWSSDKforJavav2-787c575.json new file mode 100644 index 000000000000..e25d6a91fbb4 --- /dev/null +++ b/.changes/next-release/feature-AWSSDKforJavav2-787c575.json @@ -0,0 +1,6 @@ +{ + "type": "feature", + "category": "AWS SDK for Java v2", + "contributor": "", + "description": "Make Transfer Manager work by default with MultipartS3AsyncClient." +} From 0dedeecaa7b7efe97167274650ced2d48424ca94 Mon Sep 17 00:00:00 2001 From: Olivier Lepage-Applin Date: Wed, 21 Feb 2024 09:11:28 -0500 Subject: [PATCH 3/5] TODO formatting --- .../awssdk/transfer/s3/internal/GenericS3TransferManager.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/internal/GenericS3TransferManager.java b/services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/internal/GenericS3TransferManager.java index ecc62def60b6..63d3d3ef0285 100644 --- a/services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/internal/GenericS3TransferManager.java +++ b/services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/internal/GenericS3TransferManager.java @@ -510,7 +510,7 @@ private static boolean isMrapArn(Arn arn) { return !s3EndpointResource.region().isPresent(); } - // todo remove once MultipartS3AsyncClient is complete + // TODO remove once MultipartS3AsyncClient is complete private CompletableFuture doGetObject( GetObjectRequest getObjectRequest, AsyncResponseTransformer asyncResponseTransformer) { S3AsyncClient clientToUse = s3AsyncClient; From 539b9cc99e9d46f5ce18472622595f84df1a06e2 Mon Sep 17 00:00:00 2001 From: Olivier Lepage-Applin Date: Wed, 21 Feb 2024 17:24:27 -0500 Subject: [PATCH 4/5] update changelog and rename future variables --- .../feature-AWSSDKforJavav2-787c575.json | 4 +-- .../s3/internal/GenericS3TransferManager.java | 36 +++++++++---------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/.changes/next-release/feature-AWSSDKforJavav2-787c575.json b/.changes/next-release/feature-AWSSDKforJavav2-787c575.json index e25d6a91fbb4..840c545b4531 100644 --- a/.changes/next-release/feature-AWSSDKforJavav2-787c575.json +++ b/.changes/next-release/feature-AWSSDKforJavav2-787c575.json @@ -1,6 +1,6 @@ { "type": "feature", - "category": "AWS SDK for Java v2", + "category": "S3 Transfer Manager", "contributor": "", - "description": "Make Transfer Manager work by default with MultipartS3AsyncClient." + "description": "Make Transfer Manager work by default with S3AsyncClient when multipart configuration is enabled." } diff --git a/services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/internal/GenericS3TransferManager.java b/services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/internal/GenericS3TransferManager.java index 63d3d3ef0285..ba10ac39e79e 100644 --- a/services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/internal/GenericS3TransferManager.java +++ b/services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/internal/GenericS3TransferManager.java @@ -111,11 +111,11 @@ class GenericS3TransferManager implements S3TransferManager { } @SdkTestInternalApi - GenericS3TransferManager(S3AsyncClient s3CrtAsyncClient, + GenericS3TransferManager(S3AsyncClient s3AsyncClient, UploadDirectoryHelper uploadDirectoryHelper, TransferManagerConfiguration configuration, DownloadDirectoryHelper downloadDirectoryHelper) { - this.s3AsyncClient = s3CrtAsyncClient; + this.s3AsyncClient = s3AsyncClient; this.isDefaultS3AsyncClient = false; this.transferConfiguration = configuration; this.uploadDirectoryHelper = uploadDirectoryHelper; @@ -139,13 +139,13 @@ public Upload upload(UploadRequest uploadRequest) { try { assertNotUnsupportedArn(uploadRequest.putObjectRequest().bucket(), "upload"); - CompletableFuture crtFuture = + CompletableFuture future = s3AsyncClient.putObject(uploadRequest.putObjectRequest(), requestBody); - // Forward upload cancellation to CRT future - CompletableFutureUtils.forwardExceptionTo(returnFuture, crtFuture); + // Forward upload cancellation to future + CompletableFutureUtils.forwardExceptionTo(returnFuture, future); - CompletableFutureUtils.forwardTransformedResultTo(crtFuture, returnFuture, + CompletableFutureUtils.forwardTransformedResultTo(future, returnFuture, r -> CompletedUpload.builder() .response(r) .build()); @@ -299,12 +299,12 @@ public Download download(DownloadRequest downloadReq try { assertNotUnsupportedArn(downloadRequest.getObjectRequest().bucket(), "download"); - CompletableFuture crtFuture = doGetObject(downloadRequest.getObjectRequest(), responseTransformer); + CompletableFuture future = doGetObject(downloadRequest.getObjectRequest(), responseTransformer); - // Forward download cancellation to CRT future - CompletableFutureUtils.forwardExceptionTo(returnFuture, crtFuture); + // Forward download cancellation to future + CompletableFutureUtils.forwardExceptionTo(returnFuture, future); - CompletableFutureUtils.forwardTransformedResultTo(crtFuture, returnFuture, + CompletableFutureUtils.forwardTransformedResultTo(future, returnFuture, r -> CompletedDownload.builder() .result(r) .build()); @@ -341,12 +341,12 @@ private TransferProgressUpdater doDownloadFile( assertNotUnsupportedArn(downloadRequest.getObjectRequest().bucket(), "download"); - CompletableFuture crtFuture = doGetObject(downloadRequest.getObjectRequest(), responseTransformer); + CompletableFuture future = doGetObject(downloadRequest.getObjectRequest(), responseTransformer); - // Forward download cancellation to CRT future - CompletableFutureUtils.forwardExceptionTo(returnFuture, crtFuture); + // Forward download cancellation to future + CompletableFutureUtils.forwardExceptionTo(returnFuture, future); - CompletableFutureUtils.forwardTransformedResultTo(crtFuture, returnFuture, + CompletableFutureUtils.forwardTransformedResultTo(future, returnFuture, res -> CompletedFileDownload.builder() .response(res) .build()); @@ -448,13 +448,13 @@ public Copy copy(CopyRequest copyRequest) { assertNotUnsupportedArn(copyRequest.copyObjectRequest().sourceBucket(), "copy sourceBucket"); assertNotUnsupportedArn(copyRequest.copyObjectRequest().destinationBucket(), "copy destinationBucket"); - CompletableFuture crtFuture = + CompletableFuture future = s3AsyncClient.copyObject(copyRequest.copyObjectRequest()); - // Forward transfer cancellation to CRT future - CompletableFutureUtils.forwardExceptionTo(returnFuture, crtFuture); + // Forward transfer cancellation to future + CompletableFutureUtils.forwardExceptionTo(returnFuture, future); - CompletableFutureUtils.forwardTransformedResultTo(crtFuture, returnFuture, + CompletableFutureUtils.forwardTransformedResultTo(future, returnFuture, r -> CompletedCopy.builder() .response(r) .build()); From c98178a2bfed41b92aa492e8f741540916ce3abf Mon Sep 17 00:00:00 2001 From: Olivier Lepage-Applin Date: Thu, 22 Feb 2024 17:01:12 -0500 Subject: [PATCH 5/5] testing errors with InputStream and Publisher --- .../next-release/feature-S3TransferManager-2e987ba.json | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changes/next-release/feature-S3TransferManager-2e987ba.json diff --git a/.changes/next-release/feature-S3TransferManager-2e987ba.json b/.changes/next-release/feature-S3TransferManager-2e987ba.json new file mode 100644 index 000000000000..72f2cadc85c3 --- /dev/null +++ b/.changes/next-release/feature-S3TransferManager-2e987ba.json @@ -0,0 +1,6 @@ +{ + "type": "feature", + "category": "S3 Transfer Manager", + "contributor": "", + "description": "Enable multipart configuration by default when creating a new S3TranferManager instance using the .create() method" +}