steveloughran commented on code in PR #6164:
URL: https://github.com/apache/hadoop/pull/6164#discussion_r1442966873


##########
hadoop-project/pom.xml:
##########
@@ -188,6 +188,7 @@
     <surefire.fork.timeout>900</surefire.fork.timeout>
     <aws-java-sdk.version>1.12.565</aws-java-sdk.version>
     <aws-java-sdk-v2.version>2.20.160</aws-java-sdk-v2.version>
+    
<amazon-s3-encryption-client-java.version>3.1.0</amazon-s3-encryption-client-java.version>

Review Comment:
   so this isn't in the big bundle? what does it depend on transitively?



##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/impl/TestErrorTranslation.java:
##########
@@ -153,4 +156,20 @@ public void testMultiObjectExceptionFilledIn() throws 
Throwable {
         .describedAs("retry policy of MultiObjectException")
         .isFalse();
   }
+
+  @Test
+  public void testEncryptionClientExceptionExtraction() throws Throwable {

Review Comment:
   add a test for a false match: string contains the pattern looked for, but 
inner cause is some RTE, verify translation doesn't crash



##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/ErrorTranslation.java:
##########
@@ -106,6 +110,24 @@ public static IOException maybeExtractIOException(String 
path, Throwable thrown)
 
   }
 
+  /**
+   * Extracts the underlying exception from an S3EncryptionClientException.

Review Comment:
   nit: wrong class named



##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/ErrorTranslation.java:
##########
@@ -106,6 +110,24 @@ public static IOException maybeExtractIOException(String 
path, Throwable thrown)
 
   }
 
+  /**
+   * Extracts the underlying exception from an S3EncryptionClientException.
+   * @param exception amazon exception raised
+   * @return extractedException
+   */
+  public static SdkException maybeExtractSdkException(SdkException exception) {
+    SdkException extractedException = exception;
+    if (exception.toString().contains(ENCRYPTION_CLIENT_EXCEPTION)) {

Review Comment:
   1. should this be in the classname, or is it one of those things which gets 
passed down as strings?
   2. also include check for cause being instanceof SdkException so class cast 
problems don't lose stack trace of any other problem



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