Skip to content

Core: ManifestListWriter cleanup around encryption keys #18150

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
amogh-jahagirdar merged 1 commit into from
Sep 17, 2026
+19 −29
Merged

Core: ManifestListWriter cleanup around encryption keys #18150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 11 additions & 21 deletions core/src/main/java/org/apache/iceberg/ManifestListWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,22 +36,21 @@ abstract class ManifestListWriter implements FileAppender<ManifestFile> {
private final FileAppender<ManifestFile> writer;
private final StandardEncryptionManager standardEncryptionManager;
private final NativeEncryptionKeyMetadata manifestListKeyMetadata;
private final OutputFile outputFile;

@gaborkaszab gaborkaszab Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only usage of the outputFile member outside of the constructor was to create a ManifestListFile in toManifestListFile(), but then it was unused later on. Now that the functions are merged, no need for this member

@amogh-jahagirdar amogh-jahagirdar Sep 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, even if we do my proposed approach we can drop ManifestListFile. Though in my approach, we would need OutputFile, which I feel like is very reasonable and consistent with what we do for "regular" manifests

iceberg/core/src/main/java/org/apache/iceberg/ManifestWriter.java

Line 45 in f1c6206

private final OutputFile file;
. The writer needs to have some reference to an output file

private boolean closed = false;
private ManifestListFile manifestListFile;
private FileEncryptionKeys encryptionKeys;

private ManifestListWriter(
OutputFile file, EncryptionManager encryptionManager, Map<String, String> meta) {
OutputFile outputFile;
if (encryptionManager instanceof StandardEncryptionManager) {
// ability to encrypt the manifest list key is introduced for standard encryption.
this.standardEncryptionManager = (StandardEncryptionManager) encryptionManager;
EncryptedOutputFile encryptedFile = this.standardEncryptionManager.encrypt(file);
this.outputFile = encryptedFile.encryptingOutputFile();
outputFile = encryptedFile.encryptingOutputFile();
this.manifestListKeyMetadata = (NativeEncryptionKeyMetadata) encryptedFile.keyMetadata();
} else {
this.standardEncryptionManager = null;
this.outputFile = file;
outputFile = file;
this.manifestListKeyMetadata = null;
}

Expand Down Expand Up @@ -98,25 +97,16 @@ public Long nextRowId() {
return null;
}

public ManifestListFile toManifestListFile() {

@amogh-jahagirdar amogh-jahagirdar Sep 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the dependency is weird, it's what I mentioned here 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?

@amogh-jahagirdar amogh-jahagirdar Sep 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@gaborkaszab gaborkaszab Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback, @amogh-jahagirdar !

As you pointed out SnapshotProducer is the only user of toManifestList(), but in practice the ManifestListFile return type is not needed there, just the key ID from it. In fact, I'm working on dropping this whole ManifestListFile abstraction because there is nothing specific to manifest files and we need a way to handle manifest files, root manifest, snapshot files in the same way in term of encryption.

Preconditions.checkState(closed, "Cannot build ManifestListFile, writer is not closed");
if (manifestListFile == null) {
if (manifestListKeyMetadata != null && manifestListKeyMetadata.encryptionKey() != null) {
this.encryptionKeys =
standardEncryptionManager.registerKeyMetadata(
manifestListKeyMetadata.copyWithLength(writer.length()));
this.manifestListFile =
new BaseManifestListFile(outputFile.location(), encryptionKeys.fileKey().keyId());
} else {
this.manifestListFile = new BaseManifestListFile(outputFile.location(), null);
}
FileEncryptionKeys encryptionKeys() {
Preconditions.checkState(closed, "Cannot build encryption keys, writer is not closed");
if (encryptionKeys == null
&& manifestListKeyMetadata != null
&& manifestListKeyMetadata.encryptionKey() != null) {
this.encryptionKeys =
standardEncryptionManager.registerKeyMetadata(
manifestListKeyMetadata.copyWithLength(writer.length()));
}

return manifestListFile;
}

FileEncryptionKeys encryptionKeys() {
toManifestListFile();
return encryptionKeys;
}

Expand Down
3 changes: 1 addition & 2 deletions core/src/main/java/org/apache/iceberg/SnapshotProducer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,6 @@ public Snapshot apply() {
replacedRecords);
}

ManifestListFile manifestListFile = writer.toManifestListFile();
this.manifestListEncryptionKeys = writer.encryptionKeys();

@amogh-jahagirdar amogh-jahagirdar Sep 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (OutputFile) for getting rid of state in SnapshotProducer, which I think is better as as it really shouldn't be SnapshotProducer's responsibility imo.


return new BaseSnapshot(
Expand All @@ -370,7 +369,7 @@ public Snapshot apply() {
manifestList.location(),
nextRowId,
assignedRows,
manifestListFile.encryptionKeyID());
manifestListEncryptionKeys != null ? manifestListEncryptionKeys.fileKey().keyId() : null);
}

private void runValidations(Snapshot parentSnapshot) {
Expand Down
13 changes: 7 additions & 6 deletions core/src/test/java/org/apache/iceberg/TestManifestListEncryption.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())

@amogh-jahagirdar amogh-jahagirdar Sep 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.isEqualTo(manifestListFile.encryptionKeyID());
FileEncryptionKeys encryptionKeys = writer.encryptionKeys();

// First try to read without decryption
Expand All @@ -375,7 +372,11 @@ private ManifestFile writeAndReadEncryptedManifestList(EncryptionManager em) thr
io,
EncryptionTestHelpers.createEncryptionManager(
List.of(encryptionKeys.keyEncryptionKey(), encryptionKeys.fileKey())))) {
List<ManifestFile> manifests = ManifestLists.read(readingIO.newInputFile(manifestListFile));
List<ManifestFile> manifests =
ManifestLists.read(
readingIO.newInputFile(
new BaseManifestListFile(

@gaborkaszab gaborkaszab Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, #17545 will get rid of BaseManifestListFile and use a more general version of newInputFile

outputFile.location(), encryptionKeys.fileKey().keyId())));
assertThat(manifests).hasSize(1);
return manifests.get(0);
}
Expand Down
Loading