smaheshwar-pltr commented on code in PR #14752:
URL: https://github.com/apache/iceberg/pull/14752#discussion_r2587151220


##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -215,24 +207,20 @@ the table key parameter (along with existing snapshots) 
in the file, making the
               ? Integer.parseInt(dekLengthFromHMS)
               : TableProperties.ENCRYPTION_DEK_LENGTH_DEFAULT;
 
-      encryptedKeysFromMetadata = 
Optional.ofNullable(current().encryptionKeys());
+      encryptedKeys =

Review Comment:
   If we can assume that encryption properties are unchanging (aside from keys) 
and therefore do not need to be updated on refresh, there's a actually simpler 
approach that just adds metadata keys to the current EM's transient state: 
https://github.com/apache/iceberg/compare/main...smaheshwar-pltr:iceberg:sm/add-keys?expand=1.
   
   We could also go with that approach, and introduce setters in case we can't 
make that assumption. But then I suspect just re-initialising the EM on refresh 
makes more sense. Curious though for thoughts on this assumption.



##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -215,24 +207,20 @@ the table key parameter (along with existing snapshots) 
in the file, making the
               ? Integer.parseInt(dekLengthFromHMS)
               : TableProperties.ENCRYPTION_DEK_LENGTH_DEFAULT;
 
-      encryptedKeysFromMetadata = 
Optional.ofNullable(current().encryptionKeys());
+      encryptedKeys =

Review Comment:
   If we can assume that encryption properties are unchanging (aside from keys) 
and therefore do not need to be updated on refresh, there's a actually simpler 
approach that just adds metadata keys to the current EM's transient state: 
https://github.com/apache/iceberg/compare/main...smaheshwar-pltr:iceberg:sm/add-keys?expand=1.
   
   We could also go with that approach, and introduce setters in case we can't 
make that assumption. But then I suspect just re-initialising the EM on refresh 
makes more sense. 
   
   Curious though for thoughts on this assumption.



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