[GitHub] [parquet-mr] gszadovszky commented on a change in pull request #842: PARQUET-1938: Key rotation - option to get KMS details from key material

[GitBox <gi...@apache.org>]

gszadovszky commented on a change in pull request #842:
URL: https://github.com/apache/parquet-mr/pull/842#discussion_r519957562



##########
File path: parquet-hadoop/src/main/java/org/apache/parquet/crypto/keytools/KeyToolkit.java
##########
@@ -206,9 +206,10 @@ String getEncodedWrappedKEK() {
   public static void rotateMasterKeys(String folderPath, Configuration hadoopConfig)
     throws IOException, ParquetCryptoRuntimeException, KeyAccessDeniedException, UnsupportedOperationException {
 
-    if (hadoopConfig.getBoolean(KEY_MATERIAL_INTERNAL_PROPERTY_NAME, KEY_MATERIAL_INTERNAL_DEFAULT)) {
+    if (hadoopConfig.getBoolean(KEY_MATERIAL_INTERNAL_PROPERTY_NAME, false)) {
       throw new UnsupportedOperationException("Key rotation is not supported for internal key material");
     }
+    hadoopConfig.setBoolean(KEY_MATERIAL_INTERNAL_PROPERTY_NAME, false);

Review comment:
       It is not common to set a value in the hadoop conf in our code base. I would expect some comments about why this is necessary. Also, I am not sure if this would not impact the caller. If the caller gives us a hadoop Configuration object we should not modify it.




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org