Conversation
…keys-when-cleanup-metadata
…keys-when-cleanup-metadata
…keys-when-cleanup-metadata
9b32dfd to
fda6a74
Compare
fda6a74 to
f052872
Compare
|
|
||
| Set<String> encryptionKeysToRemove = | ||
| base.encryptionKeys().stream() | ||
| .map(EncryptedKey::keyId) | ||
| .filter(keyId -> !reachableKeyIds.contains(keyId)) | ||
| .collect(Collectors.toSet()); | ||
| encryptionKeysToRemove.forEach(updatedMetaBuilder::removeEncryptionKey); |
There was a problem hiding this comment.
In a similar line to: #12670
I see 2 arguments to introduce/make RemoveEncryptionKey -> RemoveEncryptionKeys. (Bulk).
Performance: We've ran into performance issues server side when expiring 150k+ snapshots in a non-bulk way. Bulking the changes fixed it. Although I don't expect RemoveEncryptionKey to be as expensive of a call as RemoveSnapshot. So perhaps a premature optimisation.
Consistency: Consistent with the other RemovePartitionSpecs and RemoveSchemas
Curious to hear thoughts.
| Set<String> reachableKeyIds = | ||
| base.encryptionKeys().stream() | ||
| .map(EncryptedKey::encryptedById) | ||
| .filter(Objects::nonNull) | ||
| .collect(Collectors.toCollection(Sets::newConcurrentHashSet)); |
There was a problem hiding this comment.
For my own understanding. Couldn't figure out why encryptedById is nullable. In what cases do we expect this to be null?
| key -> referencedKeyIds.contains(key.keyId()) || keyEncryptionKeyId.equals(key.keyId())) | ||
| .forEach(builder::addEncryptionKey); | ||
| return builder.build(); | ||
| } |
There was a problem hiding this comment.
Need this in HiveTableOperations because we currently add all encryption keys from the encryptionManager (constructed from old metadata), and we add it to the new metadata.
This method only adds the encryption keys that are still referenced within the given metadata.
1 participant
Fixes part 2 of: #16352
The implementation constructs a set of "reachable"/"referenced" encryption keys-ids. Any key that is not referenced, we delete.
A key can be referenced in 2 places:
key-idfor every snapshot. This references the key that was used to encrypt the manifest list for that snapshotencrypted-by-idfor another encryption key.