Hugo-WB commented on code in PR #18102:
URL: https://github.com/apache/iceberg/pull/18102#discussion_r4005921882
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -241,6 +244,11 @@ private TableMetadata internalApply() {
reachableSpecs.add(base.defaultSpecId());
Set<Integer> reachableSchemas = Sets.newConcurrentHashSet();
reachableSchemas.add(base.currentSchemaId());
+ Set<String> reachableKeyIds =
+ base.encryptionKeys().stream()
+ .map(EncryptedKey::encryptedById)
+ .filter(Objects::nonNull)
+ .collect(Collectors.toCollection(Sets::newConcurrentHashSet));
Review Comment:
For my own understanding. Couldn't figure out why encryptedById is nullable.
In what cases do we expect this to be null?
##########
core/src/main/java/org/apache/iceberg/encryption/EncryptionUtil.java:
##########
@@ -181,6 +185,32 @@ public static Map<String, EncryptedKey>
encryptionKeys(EncryptionManager em) {
return sem.encryptionKeys();
}
+ /** Adds referenced encryption keys and the current key encryption key to
table metadata. */
+ public static TableMetadata addEmKeysToMetadata(
+ TableMetadata metadata, EncryptionManager encryptionManager) {
+ if (!(encryptionManager instanceof StandardEncryptionManager
standardEncryptionManager)) {
+ return metadata;
+ }
+
+ Set<String> referencedKeyIds =
+ Sets.union(
+ metadata.snapshots().stream()
+ .map(Snapshot::keyId)
+ .filter(Objects::nonNull)
+ .collect(Collectors.toUnmodifiableSet()),
+ metadata.encryptionKeys().stream()
+ .map(EncryptedKey::encryptedById)
+ .filter(Objects::nonNull)
+ .collect(Collectors.toUnmodifiableSet()));
+ String keyEncryptionKeyId = standardEncryptionManager.keyEncryptionKeyID();
+ TableMetadata.Builder builder = TableMetadata.buildFrom(metadata);
+ standardEncryptionManager.encryptionKeys().values().stream()
+ .filter(
+ key -> referencedKeyIds.contains(key.keyId()) ||
keyEncryptionKeyId.equals(key.keyId()))
+ .forEach(builder::addEncryptionKey);
+ return builder.build();
+ }
Review 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.
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -270,6 +281,13 @@ private TableMetadata internalApply() {
.filter(schemaId -> !reachableSchemas.contains(schemaId))
.collect(Collectors.toSet());
updatedMetaBuilder.removeSchemas(schemasToRemove);
+
+ Set<String> encryptionKeysToRemove =
+ base.encryptionKeys().stream()
+ .map(EncryptedKey::keyId)
+ .filter(keyId -> !reachableKeyIds.contains(keyId))
+ .collect(Collectors.toSet());
+ encryptionKeysToRemove.forEach(updatedMetaBuilder::removeEncryptionKey);
Review Comment:
In a similar line to: https://github.com/apache/iceberg/pull/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.
--
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]
