Remove the use of S3NativeClient and integrate with S3Client #2976

zoewangg · 2022-01-19T00:47:54Z

Context

S3NativeClient: an S3 SDK client provided in the aws-crt-java. It handles the full request lifecycle: marshalling, signing, sending HTTP requests, retrying, unmarshalling.

S3Client: HTTP abstraction specific to S3 provided in aws-crt-java. It handles signing, sending HTTP requests and retrying.

Description

This PR removes the use of S3NativeClient and integrates with S3Client in the transfer manager. The integration is implemented by wrapping S3Client with SdkAsyncHttpClient and passing the HTTP client to the java S3AsyncClient with certain java features disabled.

S3AsyncClient.builder()
             // Disable checksum, retry policy and signer because they are handled in crt
             .serviceConfiguration(S3Configuration.builder()
             .checksumValidationEnabled(false)
             .build())
             .overrideConfiguration(o -> o.putAdvancedOption(SdkAdvancedClientOption.SIGNER,
                                                                            new NoOpSigner())
                                                         .retryPolicy(RetryPolicy.none())
             .addExecutionInterceptor(new AttachHttpAttributesExecutionInterceptor()))
             .httpClient(s3CrtAsyncHttpClient)
             .build();

This should fix #2864

Below are the public API changes:

AsyncExecuteRequest now takes additional SDK HTTP execution attributes. This is required to pass credentials, region and other s3 settings to the S3 HTTP client.

Testing

Added unit tests.
Integ tests and stability tests passed

Screenshots (if appropriate)

Types of changes

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

Checklist

License

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

http-client-spi/src/main/java/software/amazon/awssdk/http/SdkHttpExecutionAttributes.java

http-client-spi/src/main/java/software/amazon/awssdk/http/async/AsyncExecuteRequest.java

...3-transfer-manager/src/it/java/software/amazon/awssdk/transfer/s3/S3IntegrationTestBase.java

...er-manager/src/main/java/software/amazon/awssdk/transfer/s3/S3CrtResponseHandlerAdapter.java

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

dagnir

Bennett-Lynch · 2022-01-21T20:19:33Z

...ava/software/amazon/awssdk/core/internal/http/pipeline/stages/MakeAsyncHttpRequestStage.java

-                                                                .metricCollector(httpMetricCollector)
-                                                                .build();
+                                                                .metricCollector(httpMetricCollector);
+        if (context.executionAttributes().getAttribute(SDK_HTTP_EXECUTION_ATTRIBUTES) != null) {


Should we add a variant of getAttribute() that returns an Optional, so we don't have to do multiple lookups? It's a hyper-nit in terms of performance, but it would also help readability in places like this.

Yeah, it'd be nice to have another method returning optional but since overload is not possible, I'd be hesitant to introduce another method with a different name unless we have strong reasons.

Bennett-Lynch · 2022-01-21T20:24:40Z

...ava/software/amazon/awssdk/core/internal/http/pipeline/stages/MakeAsyncHttpRequestStage.java

+            executeRequestBuilder.httpExecutionAttributes(
+                context.executionAttributes()
+                       .getAttribute(SDK_HTTP_EXECUTION_ATTRIBUTES));


What if we just copied all executionAttributes to the request, rather than selectively choosing which ones to copy? Are we worried that would leak abstractions that are not relevant to the request?

Yeah, most of the execution attributes are used in the SDK core and not relevant to the HTTP request.

http-client-spi/src/main/java/software/amazon/awssdk/http/SdkHttpExecutionAttributes.java

Bennett-Lynch · 2022-01-21T20:45:01Z

http-client-spi/src/main/java/software/amazon/awssdk/http/async/AsyncExecuteRequest.java


    private AsyncExecuteRequest(BuilderImpl builder) {
        this.request = builder.request;
        this.requestContentPublisher = builder.requestContentPublisher;
        this.responseHandler = builder.responseHandler;
        this.metricCollector = builder.metricCollector;
        this.isFullDuplex = builder.isFullDuplex;
+        this.sdkHttpExecutionAttributes = builder.executionAttributesBuilder.build();


Should this be referencing a builder or an already-built object?

What do you mean? Is there any benefit of referencing an already-built object?

It seems unusual to me that we're calling builder.executionAttributesBuilder.build() rather than just builder.executionAttributes.

I see, executionAttributes is immutable, so we have to keep a executionAttributesBuilder in the Builder inner class to allow users to provide a single attribute. We could add another executionAttributes field which will be built in the AsyncExecuteRequest build() method, but I feel it's a bit overkill.

Bennett-Lynch · 2022-01-21T21:02:00Z

...er/src/main/java/software/amazon/awssdk/transfer/s3/internal/RequestDataSupplierAdapter.java

 */
 @SdkInternalApi
-public final class RequestDataSupplierAdapter implements RequestDataSupplier {
+public final class RequestDataSupplierAdapter implements HttpRequestBodyStream {


Nit: Also rename RequestDataSupplierAdapter?

Bennett-Lynch · 2022-01-21T21:39:52Z

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

+
+    @Override
+    public String clientName() {
+        return "s3crt";


Does this follow our normal naming convention? I see other names like NettyNio, AwsCommonRuntime, Apache, etc.

This is used in the user agent header. Per the new standard, we should make it as short as possible and lower case

Bennett-Lynch · 2022-01-21T21:45:56Z

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

+        // Add the rest of the Headers
+        sdkRequest.headers().forEach((key, value) -> value.stream().map(val -> new HttpHeader(key, val))
+                                                          .forEach(crtHeaderList::add));


Is this not going to result in adding duplicates for host/content-length?

No, host and content-length will only be added if they are not present in the sdkRequest headers

Bennett-Lynch · 2022-01-21T21:50:45Z

...r/src/main/java/software/amazon/awssdk/transfer/s3/internal/S3CrtResponseHandlerAdapter.java

+        }
+    }
+
+    private static boolean isErrorResponse(int responseStatus) {


Should this be checking crtCode or responseStatus? I would assume crtCode would be non-zero based, while responseStatus would be HTTP status code based?

isErrorResponse is more of if there is an error response from the service. Apparently, responseStatus == 0 means the HTTP request itself fails.

Bennett-Lynch · 2022-01-21T21:51:22Z

...r/src/main/java/software/amazon/awssdk/transfer/s3/internal/S3CrtResponseHandlerAdapter.java

+
+    private void handleError(int crtCode, int responseStatus, byte[] errorPayload) {
+        if (isErrorResponse(responseStatus) && errorPayload != null) {
+            publisher.deliverData(ByteBuffer.wrap(errorPayload));


Is it safe to assume the byte[] won't be reused?

Yeah, I don't think it will be reused by CRT

sonarqubecloud · 2022-01-24T21:10:00Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
21 Code Smells

73.4% Coverage
0.0% Duplication

…2976)" This reverts commit 65f68be.

…3Client (#2976)"" This reverts commit cf0d833.

* Revert "Revert "Remove the use of S3NativeClient and integrate with S3Client (#2976)"" This reverts commit cf0d833. * Fix region issue

…240fc58e8 Pull request: release <- staging/47dc0a16-ab58-44a9-9b5e-36a240fc58e8

zoewangg requested a review from a team as a code owner January 19, 2022 00:47

Remove the use of S3NativeClient and integrate with S3Client

1e593ae

zoewangg force-pushed the zoewang/tm-removeS3NativeClient branch from 0e48aac to 1e593ae Compare January 19, 2022 03:09

dagnir reviewed Jan 19, 2022

View reviewed changes

zoewangg added 2 commits January 19, 2022 13:34

Fix checkstyle and build

518a0e8

Address comments and add more tests

f501b35

aceeric mentioned this pull request Jan 20, 2022

Custom ports in the S3Client awslabs/aws-crt-java#431

Closed

Rename getter sdkHttpExecutionAttributes to httpExecutionAttributes

2e4e161

dagnir approved these changes Jan 21, 2022

View reviewed changes

Bennett-Lynch approved these changes Jan 21, 2022

View reviewed changes

zoewangg added 2 commits January 21, 2022 15:20

Merge branch 'master' into zoewang/tm-removeS3NativeClient

32cc11e

Addressing feedback

94767f2

Bennett-Lynch approved these changes Jan 21, 2022

View reviewed changes

zoewangg added 3 commits January 24, 2022 10:34

Merge branch 'master' into zoewang/tm-removeS3NativeClient

9791573

Merge branch 'master' into zoewang/tm-removeS3NativeClient

fc31bb8

Merge branch 'master' into zoewang/tm-removeS3NativeClient

51cc84e

zoewangg merged commit 65f68be into master Jan 24, 2022

zoewangg deleted the zoewang/tm-removeS3NativeClient branch January 24, 2022 21:48

aws-sdk-java-automation pushed a commit that referenced this pull request Jan 25, 2022

Revert "Remove the use of S3NativeClient and integrate with S3Client (#…

cf0d833

…2976)" This reverts commit 65f68be.

zoewangg added a commit that referenced this pull request Jan 25, 2022

Revert "Revert "Remove the use of S3NativeClient and integrate with S…

8bbdb62

…3Client (#2976)"" This reverts commit cf0d833.

zoewangg added a commit that referenced this pull request Jan 26, 2022

Remove the use of S3NativeClient and integrate with S3Client (#2990)

c727e98

* Revert "Revert "Remove the use of S3NativeClient and integrate with S3Client (#2976)"" This reverts commit cf0d833. * Fix region issue

aws-sdk-java-automation added a commit that referenced this pull request Apr 12, 2024

Merge pull request #2976 from aws/staging/47dc0a16-ab58-44a9-9b5e-36a…

d52ae01

…240fc58e8 Pull request: release <- staging/47dc0a16-ab58-44a9-9b5e-36a240fc58e8

Remove the use of S3NativeClient and integrate with S3Client #2976

Remove the use of S3NativeClient and integrate with S3Client #2976

Uh oh!

Conversation

zoewangg commented Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Description

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dagnir left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoewangg Jan 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bennett-Lynch Jan 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jan 24, 2022

Uh oh!

Uh oh!

zoewangg commented Jan 19, 2022 •

edited

Loading

zoewangg Jan 21, 2022 •

edited

Loading

Bennett-Lynch Jan 21, 2022 •

edited

Loading