Hugo-WB commented on code in PR #16353:
URL: https://github.com/apache/iceberg/pull/16353#discussion_r4005679877
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -245,18 +244,7 @@ protected void doCommit(TableMetadata base, TableMetadata
metadata) {
String newMetadataLocation;
EncryptionManager encrManager = encryption();
- if (encrManager instanceof StandardEncryptionManager) {
- // Add new encryption keys to the metadata
- TableMetadata.Builder builder = TableMetadata.buildFrom(metadata);
- for (Map.Entry<String, EncryptedKey> entry :
- EncryptionUtil.encryptionKeys(encrManager).entrySet()) {
- builder.addEncryptionKey(entry.getValue());
- }
-
- tableMetadata = builder.build();
- } else {
- tableMetadata = metadata;
- }
+ tableMetadata = EncryptionUtil.addEmKeysToMetadata(metadata, encrManager);
Review Comment:
Sorry should've added a PR comment. Without this change the tests in
`TestTableEncryption` would not pass. The previous HiveTableOperations
constructed the `EncryptionManager` from the old metadata, then added all old
encryption keys to the new metadata. Thus it was impossible to ever actually
remove encryption keys.
Agree this is very confusing. Happy to discuss separately. But since
EncryptionManager stores it's own state separately to TableMetadata, it needs
to be kept in sync with the TableMetadata.
--
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]
