ahmarsuhail commented on code in PR #6884:
URL: https://github.com/apache/hadoop/pull/6884#discussion_r1718426553
##########
hadoop-tools/hadoop-aws/pom.xml:
##########
@@ -511,6 +521,11 @@
<artifactId>bundle</artifactId>
<scope>compile</scope>
</dependency>
+ <dependency>
+ <groupId>software.amazon.encryption.s3</groupId>
+ <artifactId>amazon-s3-encryption-client-java</artifactId>
+ <scope>compile</scope>
Review Comment:
hmm I thought we decided to make this provided scope?
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/ClientManagerImpl.java:
##########
@@ -128,6 +143,17 @@ private CallableRaisingIOE<S3AsyncClient>
createAyncClient() {
() -> clientFactory.createS3AsyncClient(getUri(),
clientCreationParameters));
}
+ /**
+ * Create the function to create the unencrypted S3 client.
+ * @return a callable which will create the client.
+ */
+ private CallableRaisingIOE<S3Client> createUnencryptedS3Client() {
+ return trackDurationOfOperation(
+ durationTrackerFactory,
+ STORE_CLIENT_CREATION.getSymbol(),
+ () -> unencryptedClientFactory.createS3Client(getUri(),
clientCreationParameters));
Review Comment:
can this throw a NPE? It looks like unencryptedClientFactory is null when
not using `CSEV1CompatibleS3AFileSystemHandler`, what happens then?
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/DefaultS3ClientFactory.java:
##########
@@ -153,11 +154,16 @@ public S3AsyncClient createS3AsyncClient(
.thresholdInBytes(parameters.getMultiPartThreshold())
.build();
- return configureClientBuilder(S3AsyncClient.builder(), parameters, conf,
bucket)
- .httpClientBuilder(httpClientBuilder)
- .multipartConfiguration(multipartConfiguration)
- .multipartEnabled(parameters.isMultipartCopy())
- .build();
+ S3AsyncClientBuilder s3AsyncClientBuilder =
+ configureClientBuilder(S3AsyncClient.builder(), parameters, conf,
bucket)
+ .httpClientBuilder(httpClientBuilder);
+
+ if (!parameters.isClientSideEncryptionEnabled()) {
Review Comment:
can you add a TODO and create a JIRA to remove this once the SDK issue is
fixed, seems like they are working on it
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/ClientManagerImpl.java:
##########
@@ -153,6 +179,18 @@ public synchronized S3AsyncClient getOrCreateAsyncClient()
throws IOException {
return s3AsyncClient.eval();
}
+ /**
+ * Get or create an unencrypted S3 client.
+ * This is used for unencrypted operations when CSE is enabled.
Review Comment:
nit: update doc to say it's only used when V1 compatibility is required
because ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
