RussellSpitzer commented on code in PR #17984:
URL: https://github.com/apache/iceberg/pull/17984#discussion_r3992363833
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -354,6 +360,11 @@ public Snapshot apply() {
replacedRecords);
}
+ // Each call creates a new key; use this result for both the snapshot and
key lookup.
+ ManifestListFile manifestListFile = writer.toManifestListFile();
+ this.encryptionKeysForManifestList =
+ findEncryptionKeysForManifestList(encryption,
manifestListFile.encryptionKeyID());
Review Comment:
The reverse lookup here seems a bit backwards. We should already have these
objects and trying to find them recursively feels incorrect.
What if instead we change "addManifestListKeys" to return a list of keys and
include the kek and mlk
```java
@deprecate add a deprecation here
public String addManifestListKeyMetadata(NativeEncryptionKeyMetadata
keyMetadata) {
return addManifestListKeys(keyMetadata).get(1).keyId(); // MLK
}
public List<EncryptedKey> addManifestListKeys(NativeEncryptionKeyMetadata
keyMetadata) {
String manifestListKeyID = generateKeyId();
String keyEncryptionKeyID = keyEncryptionKeyID();
EncryptedKey kek = encryptionKeys.get(keyEncryptionKeyID); // already in
the map
ByteBuffer encryptedKeyMetadata =
EncryptionUtil.encryptManifestListKeyMetadata(...);
EncryptedKey mlk =
new BaseEncryptedKey(manifestListKeyID, encryptedKeyMetadata,
keyEncryptionKeyID, null);
encryptionKeys.put(mlk.keyId(), mlk);
return List.of(kek, mlk);
}
```
Then we have toManifestListFile() store both of these keys and return them
in encryptionKeys
```java
private ManifestListFile manifestListFile;
private List<EncryptedKey> encryptionKeys = List.of();
public ManifestListFile toManifestListFile() {
if (manifestListFile == null) {
if (manifestListKeyMetadata != null &&
manifestListKeyMetadata.encryptionKey() != null) {
List<EncryptedKey> keys =
standardEncryptionManager.addManifestListKeys(
manifestListKeyMetadata.copyWithLength(writer.length()));
this.encryptionKeys = keys;
this.manifestListFile =
new BaseManifestListFile(outputFile.location(),
keys.get(1).keyId());
} else {
this.manifestListFile = new
BaseManifestListFile(outputFile.location(), null);
}
}
return manifestListFile;
}
List<EncryptedKey> encryptionKeys() {
toManifestListFile();
return encryptionKeys;
}
```
Then snapshot producer just gets
```
this.encryptionKeysForManifestList = writer.encryptionKeys();
```
--
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]
