gaborkaszab commented on code in PR #16353:
URL: https://github.com/apache/iceberg/pull/16353#discussion_r4004701530
##########
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:
I'm not entirely sure what problem this part of the PR tries to solve. I had
the impression that the current changes in `TableMetadata` are meant to cover
the scope of the PR: "Clean up encryption keys as part of RemoveSnapshots".
This seems like another mechanism to clean up unreferenced encryption keys
when committing to a table. If I'm not mistaken here with my assumption, then
this is orthogonal and should be covered separately.
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -1451,6 +1451,7 @@ private Builder rewriteSnapshotsInternal(Collection<Long>
idsToRemove, boolean s
}
removeStatistics(snapshotId);
removePartitionStatistics(snapshotId);
+ removeEncryptionKey(snapshot.keyId());
Review Comment:
Thank you for getting back with this @Hugo-WB !
I think encryption keys is not any different than schemas or partition specs
for instance. When we suppress snapshots to REFS we still need the complete
list of schemes and specs in case we want to load later on the missing
snapshots (suppress mode). When expiring snapshot, if no remaining snapshot
uses them, they can be removed (!suppress mode).
One thing I think still breaks here: I'm not entirely sure we can assume
that the mapping between snapshots and encryption keys is 1 to 1. In the
reference implementation it seems to be the case, but there might be custom
implementations where this isn't true.
To make this more general, I think we should also account the snapshots
retained and their key IDs. Maybe adding the change not here but to
`RemoveSnapshots.internalApply()`?
--
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]
