JeonDaehong commented on code in PR #18268:
URL: https://github.com/apache/iceberg/pull/18268#discussion_r4177233836
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -1120,6 +1122,13 @@ protected void cleanUncommitted(Set<ManifestFile>
committed) {
private void cleanUncommittedAppends(Set<ManifestFile> committed) {
deleteUncommitted(cachedNewDataManifests, committed, true /* clear
manifests */);
deleteUncommitted(cachedNewDeleteManifests, committed, true /* clear
manifests */);
+ if (cachedNewDeleteManifests.isEmpty()) {
Review Comment:
I think this could delete a Puffin file that is still referenced after a
successful commit.
If the new delete manifest is merged with an existing manifest during
manifest merging, the commit succeeds, but the cached manifest is not included
in `committed`. Then `deleteUncommitted` deletes the cached manifest and clears
the list, causing this branch to delete the merged DV Puffin file even though
the merged manifest still references it.
I was able to reproduce this with `commit.manifest.min-count-to-merge=1`:
start with one existing delete manifest, then perform a `RowDelta` with two DVs
for the same data file. After the commit, the snapshot contains one delete
manifest whose DV points to `merged-dvs-...puffin`, but the Puffin file itself
has already been deleted. The same test passes on `main`.
Would it be safer to delete these files only when the commit fails (i.e.
when `committed` is empty)?
--
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]
