Performance Enhancement :Enable Passing of requestPath to CRT and monitor the progress using CRTs S3MetaRequestProgress #4379

joviegas · 2023-08-31T19:10:30Z

Motivation and Context

The File IO operation during Upload of the File should not be done in SDK side.
The requestPath is passed to the CRT and CRT does the IO operation.
CRT side changes Ability to pass request file path to S3, for faster uploads awslabs/aws-crt-java#595 and Progress callbacks fire for all meta-request types awslabs/aws-c-s3#344

Modifications

Removed the code where S3CrtRequestBodyStreamAdapter is passed to CRTHttp client
Now we just pass null is request Path is passed to the S3MetaRequest.
The Progress listener will listen to S3MetaRequestProgress from CRT

Testing

No need of new Test since this is internal behaviour test, the existing test cases should cover this.
Executed Integ tests.

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

License

I confirm that this pull request can be released under the Apache 2 license

…r Progress Updates for putObject Operations

millems

Is it possible to test that what we're intending to fix actually got fixed?
Is there performance testing to verify that this change didn't hurt performance?
Do we already have tests that assert the progress listener changes were backwards compatible?
Are there any path-processing nuances that may have changed when we switched to the CRT? E.g. handling of the current working directory or expansion of ~/?

millems · 2023-09-05T17:03:04Z

...-manager/src/main/java/software/amazon/awssdk/transfer/s3/internal/CrtS3TransferManager.java

        progressUpdater.registerCompletion(returnFuture);

        try {
            assertNotUnsupportedArn(putObjectRequest.bucket(), "upload");

            CompletableFuture<PutObjectResponse> crtFuture =
-                s3AsyncClient.putObject(putObjectRequest, requestBody);
+                s3AsyncClient.putObject(putObjectRequest, AsyncRequestBody.fromFile(uploadFileRequest.source()));


If CRT is reading directly from disk, does this publisher get used?

CRT directly gets this from file. The Publisher here is in the S3Client Interface and a non null is required just to get the content length a I have updated the code to throw an exception if we try to read the Java SDKs Publisher in such cases.

millems · 2023-09-05T17:11:55Z

.../s3/src/main/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtProgressListener.java

+/**
+ * S3CrtProgressListener delegates events to the underlying delegateListener if defined, avoiding null checks for API when using
+ * S3MetaRequestProgress for GET calls.
+ */


This feels like a weird application of polymorphism. Instead of S3CrtProgressListener, can we just have a NoOpPublisherListener that we create if delegateListener is null? That just means we need the null check in the setup code of S3CrtResponseHandlerAdapter instead of per-method here.

Created NoOpPublisherListener

millems · 2023-09-05T17:13:12Z

...ava/software/amazon/awssdk/services/s3/internal/crt/S3InternalSdkHttpExecutionAttribute.java

+    public static final S3InternalSdkHttpExecutionAttribute<Path> SOURCE_REQ_PATH =
+        new S3InternalSdkHttpExecutionAttribute<>(Path.class);
+
+
+    public static final S3InternalSdkHttpExecutionAttribute<PublisherListener> CRT_PROGRESS_LISTENER =
+        new S3InternalSdkHttpExecutionAttribute<>(PublisherListener.class);


This is marked as @SdkInternalApi, so it can't be used by the transfer manager module, like we're doing above. Do we need to move a subset to a @SdkProtectedApi?

millems · 2023-09-05T17:18:03Z

...ava/software/amazon/awssdk/services/s3/internal/crt/S3InternalSdkHttpExecutionAttribute.java

+    public static final S3InternalSdkHttpExecutionAttribute<Path> SOURCE_REQ_PATH =
+        new S3InternalSdkHttpExecutionAttribute<>(Path.class);


Naming nitpicks, since this actually seems like a protected API.

SOURCE - Do we ever expect to use this for something other than a PutObject - like a GetObject, where it's actually the DESTINATION?

REQ - We should almost always use the full word for something instead of shortening it, so REQUEST. It looks like a lot of the existing properties are request properties, though, so I don't think REQUEST is necessary.

If it's always the source, what about PUT_OBJECT_PATH? UPLOAD_PATH?
If it's potentially also destination, what about UPLOAD_DOWNLOAD_PATH?

Thanks Matt..
Named it as OBJECT_FILE_PATH so that it can be used for any file path for get/put

joviegas · 2023-09-07T16:50:00Z

Is it possible to test that what we're intending to fix actually got fixed?

Thanks Matt for this suggestion , tested this and realized the CRT doesnot actually fix the issue where upload file size is more than RAM size. I have cut a different ticket to CRT. However , this fix is still needed for performance enhancement. Removed the mention of the orginal issue.

Is there performance testing to verify that this change didn't hurt performance?

Yes , I saw that the latencies was reduced to half , I have updated the internal doc.

Do we already have tests that assert the progress listener changes were backwards compatible?

Yes , we already have test .

Are there any path-processing nuances that may have changed when we switched to the CRT? E.g. handling of the current working directory or expansion of ~/?

No.

.changes/next-release/bugfix-S3TransferManager-7ffc393.json

.../main/java/software/amazon/awssdk/transfer/s3/internal/progress/TransferProgressUpdater.java

...-manager/src/main/java/software/amazon/awssdk/transfer/s3/internal/CrtS3TransferManager.java

.../src/it/java/software/amazon/awssdk/services/s3/crt/S3CrtClientPutObjectIntegrationTest.java

...a/software/amazon/awssdk/services/s3/internal/crt/S3TmInternalSdkHttpExecutionAttribute.java

zoewangg · 2023-09-07T21:14:32Z

...c/main/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtResponseHandlerAdapter.java

@@ -46,9 +48,14 @@ public final class S3CrtResponseHandlerAdapter implements S3MetaRequestResponseH
    private final SdkHttpResponse.Builder respBuilder = SdkHttpResponse.builder();
    private volatile S3MetaRequest metaRequest;

-    public S3CrtResponseHandlerAdapter(CompletableFuture<Void> executeFuture, SdkAsyncHttpResponseHandler responseHandler) {
+    private final PublisherListener<S3MetaRequestProgress> progressListener;


Do we need to invoke this.progressListener.subscriberOnError(); in onErrorResponseComplete?

We are already calling this.progressListener.subscriberOnError(exception); from failResponseHandlerAndFuture which is called from errorPayload != null also errorPayload == null for valid errorPayloads

failResponseHandlerAndFuture doesn't seem to be invoked from onErrorResponseComplete if it successfully marshalls the error response though. Can we add a test to verify it? This test may not suffice now that we are relying on the real CRT client to report progress and it is using a mock s3 client.

responsePublisher.send(ByteBuffer.wrap(errorPayload)) .thenRun(responsePublisher::complete) .handle((ignore, throwable) -> { if (throwable != null) { failResponseHandlerAndFuture(throwable); return null; } completeFutureAndCloseRequest(); return null; });

Added new mock tests and removed explicit onError

...oftware/amazon/awssdk/services/s3/internal/crt/CrtContentLengthOnlyAsyncFileRequestBody.java

zoewangg · 2023-09-07T23:33:52Z

.../main/java/software/amazon/awssdk/transfer/s3/internal/progress/TransferProgressUpdater.java

@@ -107,8 +107,7 @@ public void publisherSubscribe(Subscriber<? super S3MetaRequestProgress> subscri

            @Override
            public void subscriberOnNext(S3MetaRequestProgress s3MetaRequestProgress) {
-                incrementBytesTransferred(Math.toIntExact(s3MetaRequestProgress.getBytesTransferred()),
-                                          s3MetaRequestProgress.getContentLength());
+                incrementBytesTransferred(Math.toIntExact(s3MetaRequestProgress.getBytesTransferred()));


We probably should not cast it to int. Can we change incrementBytesTransferred to take a long instead?

...oftware/amazon/awssdk/services/s3/internal/crt/CrtContentLengthOnlyAsyncFileRequestBody.java

zoewangg · 2023-09-07T23:56:05Z

...c/main/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtResponseHandlerAdapter.java

@@ -46,9 +48,14 @@ public final class S3CrtResponseHandlerAdapter implements S3MetaRequestResponseH
    private final SdkHttpResponse.Builder respBuilder = SdkHttpResponse.builder();
    private volatile S3MetaRequest metaRequest;

-    public S3CrtResponseHandlerAdapter(CompletableFuture<Void> executeFuture, SdkAsyncHttpResponseHandler responseHandler) {
+    private final PublisherListener<S3MetaRequestProgress> progressListener;


failResponseHandlerAndFuture doesn't seem to be invoked from onErrorResponseComplete if it successfully marshalls the error response though. Can we add a test to verify it? This test may not suffice now that we are relying on the real CRT client to report progress and it is using a mock s3 client.

responsePublisher.send(ByteBuffer.wrap(errorPayload)) .thenRun(responsePublisher::complete) .handle((ignore, throwable) -> { if (throwable != null) { failResponseHandlerAndFuture(throwable); return null; } completeFutureAndCloseRequest(); return null; });

sonarqubecloud · 2023-09-11T23:50:11Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
9 Code Smells

55.9% Coverage
0.0% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

joviegas requested a review from a team as a code owner August 31, 2023 19:10

joviegas changed the title ~~Performance Enhancement :Enable Passing of requestPath to CRT, Monitor Progress Updates for putObject Operations~~ Performance Enhancement :Enable Passing of requestPath to CRT and monitor the progress using CRTs S3MetaRequestProgress Aug 31, 2023

joviegas force-pushed the joviegas/crt_upload_request_path branch 3 times, most recently from 63065aa to 6c97310 Compare August 31, 2023 21:15

Performance Enhancement :Enable Passing of requestPath to CRT, Monito…

0e28441

…r Progress Updates for putObject Operations

joviegas force-pushed the joviegas/crt_upload_request_path branch from 6c97310 to 0e28441 Compare September 1, 2023 15:30

millems reviewed Sep 5, 2023

View reviewed changes

joviegas added 3 commits September 5, 2023 16:55

Merge branch 'master' into joviegas/crt_upload_request_path

35fec51

Merge branch 'master' into joviegas/crt_upload_request_path

144c10b

updated the change log

66467b5

joviegas force-pushed the joviegas/crt_upload_request_path branch from 58fabbd to e047e6c Compare September 7, 2023 16:42

zoewangg reviewed Sep 7, 2023

View reviewed changes

...oftware/amazon/awssdk/services/s3/internal/crt/CrtContentLengthOnlyAsyncFileRequestBody.java Show resolved Hide resolved

updated with Matts review comments

6af3397

joviegas force-pushed the joviegas/crt_upload_request_path branch from e047e6c to 6af3397 Compare September 7, 2023 23:25

zoewangg reviewed Sep 7, 2023

View reviewed changes

Added Integ Test for CRT Progress listener updates

65ddbca

joviegas force-pushed the joviegas/crt_upload_request_path branch from 0791331 to 65ddbca Compare September 8, 2023 16:50

Test cases to check on Failure

137862e

joviegas force-pushed the joviegas/crt_upload_request_path branch from 9247f21 to 137862e Compare September 8, 2023 22:15

zoewangg approved these changes Sep 8, 2023

View reviewed changes

Updated the crt version to lattest one

99b7736

joviegas force-pushed the joviegas/crt_upload_request_path branch from 8df43eb to 1cac2de Compare September 10, 2023 00:18

Merge branch 'master' into joviegas/crt_upload_request_path

aaa43a0

joviegas force-pushed the joviegas/crt_upload_request_path branch from 1cac2de to aaa43a0 Compare September 11, 2023 19:25

joviegas added 3 commits September 11, 2023 12:31

removed the Findbug error

0b88836

Merge branch 'master' into joviegas/crt_upload_request_path

cce960e

Added delay in Test so that listeners are updated parallely

152d896

joviegas force-pushed the joviegas/crt_upload_request_path branch from 5d6b70e to 152d896 Compare September 11, 2023 22:14

joviegas merged commit d59470c into master Sep 12, 2023
6 of 7 checks passed

joviegas deleted the joviegas/crt_upload_request_path branch May 15, 2024 20:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance Enhancement :Enable Passing of requestPath to CRT and monitor the progress using CRTs S3MetaRequestProgress #4379

Performance Enhancement :Enable Passing of requestPath to CRT and monitor the progress using CRTs S3MetaRequestProgress #4379

joviegas commented Aug 31, 2023 •

edited

Loading

millems left a comment •

edited

Loading

millems Sep 5, 2023

joviegas Sep 7, 2023

millems Sep 5, 2023

joviegas Sep 6, 2023

millems Sep 5, 2023

joviegas Sep 7, 2023

millems Sep 5, 2023

joviegas Sep 7, 2023

joviegas commented Sep 7, 2023

zoewangg Sep 7, 2023

joviegas Sep 7, 2023

zoewangg Sep 7, 2023

joviegas Sep 8, 2023

zoewangg Sep 7, 2023

zoewangg Sep 7, 2023

sonarqubecloud bot commented Sep 11, 2023

		public static final S3InternalSdkHttpExecutionAttribute<Path> SOURCE_REQ_PATH =
		new S3InternalSdkHttpExecutionAttribute<>(Path.class);

Performance Enhancement :Enable Passing of requestPath to CRT and monitor the progress using CRTs S3MetaRequestProgress #4379

Performance Enhancement :Enable Passing of requestPath to CRT and monitor the progress using CRTs S3MetaRequestProgress #4379

Conversation

joviegas commented Aug 31, 2023 • edited Loading

Motivation and Context

Modifications

Testing

Screenshots (if appropriate)

Types of changes

License

millems left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joviegas commented Sep 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Sep 11, 2023

joviegas commented Aug 31, 2023 •

edited

Loading

millems left a comment •

edited

Loading