gaborkaszab commented on code in PR #17984:
URL: https://github.com/apache/iceberg/pull/17984#discussion_r4034572466
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -353,6 +356,9 @@ public Snapshot apply() {
replacedRecords);
}
+ ManifestListFile manifestListFile = writer.toManifestListFile();
Review Comment:
I think we introduced a weird dependency between `toManifestFile` and
`encryptionKey` and it is apparent here. The latter calls the former, while
both return something where the return value of the former seems to be a subset
of the return value of the latter.
With this design, I don't think `toManifestFile` should return anything.
Also the name is incorrect: the purpose of that function is to register key
metadata to an encryption manager. The only think we use from the output of
`toManifestFile` is the keyId, but that also seems present in the output of
`encryptionKeys`, so these seem redundant.
Also, since `encryptionKeys` internally calls `toManifestFile()` I think we
can avoid calling `toManifestFile` here.
--
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]
