HADOOP-19399. Adds in support for CRT client. #7443
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
ahmarsuhail
commented
Mar 3, 2025
| @@ -0,0 +1,70 @@ | |||
| package org.apache.hadoop.fs.s3a.impl; | |||
|
|
|||
There was a problem hiding this comment.
@steveloughran starting to work on the CRT changes, wanted to get your opinion on this. What do you think about doing some refactoring around this region logic, so we have our own builder and container class for AWSRegionEndpointInformation which all clients can get information out of.
Without this you have to duplicate the endpoint region logic for CRT and the other clients since they use different builder classes. CRT uses S3CrtAsyncClientBuilder which does not extend S3BaseClientBuilder and so you can't use the existing method..
|
💔 -1 overall
This message was automatically generated. |
ahmarsuhail
force-pushed
the
HADOOP-19399-crt-client-support
branch
from
badf0db to
032c9a8
Compare
ahmarsuhail
commented
Mar 7, 2025
| @@ -324,6 +326,19 @@ | |||
| </properties> | |||
| </profile> | |||
|
|
|||
| <!-- Use the S3 CRT client --> | |||
There was a problem hiding this comment.
this doesn't seem to be working btw, ran mvn clean verify -Dit.test=ITestAwsSdkWorkarounds -Dtest=none -Pcrt as I expect ITestAwsSdkWorkarounds to fail when using CRT, but it still passes. Need to look
ahmarsuhail
commented
Mar 7, 2025
| @@ -356,7 +362,7 @@ public PutObjectRequest.Builder newPutObjectRequestBuilder(String key, | |||
| } | |||
|
|
|||
| // Set the timeout for object uploads but not directory markers. | |||
| if (!isDirectoryMarker) { | |||
| if (!isDirectoryMarker && !isCRTEnabled) { | |||
There was a problem hiding this comment.
currently cannot set request level timeouts with CRT
|
@steveloughran @mukund-thakur @shameersss1 Could you please review? Internal TPC-DS benchmarks for AAL seem to be consistently showing better numbers with CRT, possibly due to the CRT's enhanced connection management, so want to get this in for 3.4.2. |
ahmarsuhail
commented
Mar 7, 2025
| durationTrackerFactory, | ||
| STORE_CLIENT_CREATION.getSymbol(), | ||
| () -> clientFactory.createS3AsyncClient(getUri(), clientCreationParameters)); | ||
| return trackDurationOfOperation(durationTrackerFactory, STORE_CLIENT_CREATION.getSymbol(), |
There was a problem hiding this comment.
this should actually go into the client factory, not here. moving this over
rajdchak
reviewed
Mar 7, 2025
| if (regionEndpointInformation.getEndpoint() == null) { | ||
| s3CrtAsyncClientBuilder.endpointOverride(regionEndpointInformation.getEndpoint()); | ||
| } | ||
|
|
There was a problem hiding this comment.
This doesn't look right. We are doing a null check for both the region and the endpoint and then setting to that if null?
There was a problem hiding this comment.
thanks, it was not right :) fixed!
rajdchak
reviewed
Mar 7, 2025
| @@ -0,0 +1,232 @@ | |||
| /* | |||
There was a problem hiding this comment.
Can we not modify the configureEndpointAndRegion method in DefaultS3ClientFactory class to support AWSRegionEndpointInformation and not create a new class for this?
There was a problem hiding this comment.
prefer to move it out to a separate class, this logic doesn't really belong in the client factory imo
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
Adds support S3 CRT client.
Configuration mapping b/w clients is getting tricky, I moved endpoint/region resolution logic out so it can be re-used.
Still TODO:
How was this patch tested?
Tested in us-east-2, all good, except
since we're using the CRT client, the
The provided S3AsyncClient is an instance of MultipartS3AsyncClientno longer gets logged. This is also not logged in the latest SDKs! so maybe we cut this test now?For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?