Hugo-WB commented on code in PR #16353:
URL: https://github.com/apache/iceberg/pull/16353#discussion_r4005750655
##########
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:
Agreed! In the current spec encryption keys are stored at the metadata
layer, similar to schemas/partition specs. I think this makes sense for the
KEK, however for manifest list encryption key, those are per snapshot and not
re-used, so would've been nice to have them per snapshot.
As you mention above having the keys in the snapshot itself would also make
the invariant of 1:1 snapshot <> encryption key more obvious.
Yep! I had this PR open that I was planning on opening up after this one
merged: https://github.com/apache/iceberg/pull/16395/changes which creates the
list of references keys and removes all unreferenced.
Agreed on the invariance, I mentioned it in the PR description. It seems
like things that are 1:1 with snapshot are removed at
`TableMetadata::rewriteSnapshotsInternal` and things that are shared across
snapshots (schemas/partitions etc...)
`RemoveSnapshots::internalApply::cleanExpiredMetadata`.
If we rely on this invariance, which I think people are generally agreed
with relying on this invariant/it holding in the long run (Mentioned it in PR
description and didn't get any disagreements)? I think it makes sense to remove
encryption keys as part of `TableMetadata::rewriteSnapshotsInternal`.
Thanks! Curious on your thoughts.
--
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]
