amogh-jahagirdar commented on code in PR #17984:
URL: https://github.com/apache/iceberg/pull/17984#discussion_r4018922664
##########
core/src/main/java/org/apache/iceberg/ManifestListWriter.java:
##########
@@ -94,14 +99,26 @@ public Long nextRowId() {
}
public ManifestListFile toManifestListFile() {
- if (manifestListKeyMetadata != null &&
manifestListKeyMetadata.encryptionKey() != null) {
- String manifestListKeyID =
- standardEncryptionManager.addManifestListKeyMetadata(
- manifestListKeyMetadata.copyWithLength(writer.length()));
- return new BaseManifestListFile(outputFile.location(),
manifestListKeyID);
- } else {
- return new BaseManifestListFile(outputFile.location(), null);
+ Preconditions.checkState(closed, "Cannot build ManifestListFile, writer is
not closed");
+ if (manifestListFile == null) {
+ if (manifestListKeyMetadata != null &&
manifestListKeyMetadata.encryptionKey() != null) {
+ this.encryptionKeys =
Review Comment:
Can we initialize the encryptionKeys field inside close()? It was a bit
weird to see that encryptionKeys() relies on state produced by
toManifestListFile().
##########
core/src/main/java/org/apache/iceberg/ManifestListWriter.java:
##########
@@ -94,14 +99,26 @@ public Long nextRowId() {
}
public ManifestListFile toManifestListFile() {
- if (manifestListKeyMetadata != null &&
manifestListKeyMetadata.encryptionKey() != null) {
- String manifestListKeyID =
- standardEncryptionManager.addManifestListKeyMetadata(
- manifestListKeyMetadata.copyWithLength(writer.length()));
- return new BaseManifestListFile(outputFile.location(),
manifestListKeyID);
- } else {
- return new BaseManifestListFile(outputFile.location(), null);
+ Preconditions.checkState(closed, "Cannot build ManifestListFile, writer is
not closed");
+ if (manifestListFile == null) {
+ if (manifestListKeyMetadata != null &&
manifestListKeyMetadata.encryptionKey() != null) {
+ this.encryptionKeys =
+ standardEncryptionManager.registerManifestListKeyMetadata(
+ manifestListKeyMetadata.copyWithLength(writer.length()));
+ this.manifestListFile =
+ new BaseManifestListFile(
+ outputFile.location(),
encryptionKeys.manifestListKey().keyId());
+ } else {
+ this.manifestListFile = new
BaseManifestListFile(outputFile.location(), null);
+ }
}
+
+ return manifestListFile;
+ }
+
+ ManifestListEncryptionKeys encryptionKeys() {
+ toManifestListFile();
Review Comment:
See comment above, I feel like encryptionKeys should just verify that it's
closed, and close() should actually be setting the encryptionKeys field rather
than toManifestListFIle. Feels weird to read "toManifestListFile" here just so
that the state is correct
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -499,10 +540,14 @@ public void commit() {
if (base.snapshot(newSnapshot.snapshotId()) != null) {
// this is a rollback operation
update.setBranchSnapshot(newSnapshot.snapshotId(),
targetBranch);
- } else if (stageOnly) {
- update.addSnapshot(newSnapshot);
} else {
- update.setBranchSnapshot(newSnapshot, targetBranch);
+ // Ensure the snapshot's manifest list key and its
wrapping key are present.
+
encryptionKeysForManifestList.forEach(update::addEncryptionKey);
+ if (stageOnly) {
+ update.addSnapshot(newSnapshot);
+ } else {
+ update.setBranchSnapshot(newSnapshot, targetBranch);
+ }
}
Review Comment:
I'm not sure I agree this is a "very hot code path" in the sense that even
for streaming commits where it's a commit every 30 seconds (or even take even
more extreme cases), we're basically doing 2 map.put operations (unless I'm
missing something), which in relation to all the I/O happening on a commit is
pretty nominal.
I think the code is correct as is unless I'm missing something; if we were
to move the code out for both cases, in the "if" case (the rollback branch), I
think we'd be potentially adding new keys that are effectivley orphaned.
--
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]
