ahmarsuhail commented on code in PR #6507:
URL: https://github.com/apache/hadoop/pull/6507#discussion_r1472858212


##########
hadoop-project/pom.xml:
##########
@@ -187,7 +187,7 @@
     <make-maven-plugin.version>1.0-beta-1</make-maven-plugin.version>
     <surefire.fork.timeout>900</surefire.fork.timeout>
     <aws-java-sdk.version>1.12.599</aws-java-sdk.version>
-    <aws-java-sdk-v2.version>2.23.5</aws-java-sdk-v2.version>

Review Comment:
   cut, SDK upgrade needs to happen separately



##########
hadoop-tools/hadoop-aws/pom.xml:
##########
@@ -508,6 +508,29 @@
       <artifactId>bundle</artifactId>
       <scope>compile</scope>
     </dependency>
+    <dependency>

Review Comment:
   @steveloughran do you have any advice here? I think we should do what we did 
for Client Side Encryption, have this S3 access grants jar as optional, and 
create a new client factory which will add the S3 access grants plugin. 
   
   If there are other plugins that we want to add in the future, this new 
client factory can be generalised for that. 



##########
hadoop-tools/hadoop-aws/pom.xml:
##########
@@ -508,6 +508,29 @@
       <artifactId>bundle</artifactId>
       <scope>compile</scope>
     </dependency>
+    <dependency>

Review Comment:
   this should go in hadoop-project, and then set the dependency here. We 
should probably also use `provided` scope, so the jar is optional. 
   
   
   See this PR, which added the client side encryption for example 
https://github.com/apache/hadoop/pull/6164 and 
[this](https://github.com/apache/hadoop/pull/6164/files#diff-c76d380f28cd282404a2b7110a6ea76bf2edd7277ed09639a2af594171b07efaR53)
 class which checks if the class exists



##########
LICENSE-binary:
##########
@@ -363,7 +363,7 @@ org.objenesis:objenesis:2.6
 org.xerial.snappy:snappy-java:1.1.10.4
 org.yaml:snakeyaml:2.0
 org.wildfly.openssl:wildfly-openssl:1.1.3.Final
-software.amazon.awssdk:bundle:jar:2.23.5

Review Comment:
   this shouldn't be here. it's already part of your SDK upgrade PR so cut from 
here



-- 
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]

Reply via email to