amogh-jahagirdar commented on code in PR #18150:
URL: https://github.com/apache/iceberg/pull/18150#discussion_r4037231321
##########
core/src/main/java/org/apache/iceberg/ManifestListWriter.java:
##########
@@ -98,25 +97,16 @@ public Long nextRowId() {
return null;
}
- public ManifestListFile toManifestListFile() {
Review Comment:
I agree that the dependency is weird, it's what I mentioned
[here](https://github.com/apache/iceberg/pull/17984#discussion_r4018922664)
already; we should clean that part up.
But I think there's a good argument for having a `toManifestListFile` API,
it's consistent with what we have for manifest files (`toManifestFile`) and I
think fundamentally a writer handing back a "This is the complete object" makes
it easier for consumers to just use that as a source of truth (e.g. they can do
manifestList.encryptionKeys(), manifestList.location() etc).
In our case the consumer is just SnapshotProducer
So what I was thinking was a good way to clean this up would be
```
EncryptionKeys encryptionKeys() {
/// what you have now
}
ManifestListFile toManifestList() {
EncryptionKeys encryptionKeys = encryptionKeys()
return new ManifestListFile(location, encryptionKeys != null ?
encryptionKeys.fileKey().keyId() : null):
}
```
So we get rid of the bad dependency ordering and still keep (imo) a useful
API.
WDYT?
##########
core/src/test/java/org/apache/iceberg/TestManifestListEncryption.java:
##########
@@ -353,15 +353,12 @@ private ManifestFile
writeAndReadEncryptedManifestList(EncryptionManager em) thr
SEQ_NUM,
SNAPSHOT_FIRST_ROW_ID);
try (writer) {
- assertThatThrownBy(writer::toManifestListFile)
+ assertThatThrownBy(writer::encryptionKeys)
.isInstanceOf(IllegalStateException.class)
- .hasMessage("Cannot build ManifestListFile, writer is not closed");
+ .hasMessage("Cannot build encryption keys, writer is not closed");
writer.add(TEST_MANIFEST);
}
- ManifestListFile manifestListFile = writer.toManifestListFile();
- assertThat(writer.toManifestListFile().encryptionKeyID())
Review Comment:
One possible issue with removing this is that then we're not effectively
testing the `encryptionKeys == null` which currently prevents registering 2
different keys for the same file. But fwiw the utility of testing this behavior
is moot if we're only calling this from snapshot producer just once. So I'm
good with the removal.
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -356,7 +356,6 @@ public Snapshot apply() {
replacedRecords);
}
- ManifestListFile manifestListFile = writer.toManifestListFile();
this.manifestListEncryptionKeys = writer.encryptionKeys();
Review Comment:
If we do what I mentioned above, I think we can effectively get rid of this
state on SnapshotProducer, and just pass in manifestListFile.encryptionKeys().
So it trades off more state in ManifestListFile for getting rid of state in
SnapshotProducer, which I think is better as SnapshotProducer has disjoint
state for encryption keys ofr manifest list and the actual manifest list which
feels unfortunate.
##########
core/src/main/java/org/apache/iceberg/ManifestListWriter.java:
##########
@@ -98,25 +97,16 @@ public Long nextRowId() {
return null;
}
- public ManifestListFile toManifestListFile() {
Review Comment:
I'm fine as it is btw, we can always add functions later, but just wanted to
point out that the toManifestListFile was something thought through and not
rushed :) but agree on cleaning up the dependency/state, that's a good thing.
--
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]
