dkranchii commented on code in PR #17745:
URL: https://github.com/apache/iceberg/pull/17745#discussion_r3831879645
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1598,6 +1598,48 @@ public void testRetainFilesOnRetainedBranches() {
assertThat(deletedFiles).isEqualTo(expectedDeletes);
}
+ @TestTemplate
+ public void testReachableCleanupWithDeleteManifestFromRemovedBranch() {
+ // Regression test: ReachableFileCleanup previously used
ManifestFiles.readPaths, which only
+ // supports DATA manifests and throws IllegalArgumentException on DELETE
manifests. When a
+ // DELETE manifest was reachable only through an expired snapshot (as
happens after a branch
+ // that added delete files is removed), the exception was logged and
swallowed by the
+ // suppressFailureWhenFinished task loop, silently orphaning the
referenced delete and DV
+ // files on object storage.
+ assumeThat(formatVersion).as("Delete files only supported in V2
spec").isEqualTo(2);
+ assumeThat(incrementalCleanup)
+ .as("This test targets ReachableFileCleanup, not
IncrementalFileCleanup")
+ .isFalse();
+
+ String branch = "delete-branch";
+
+ table.newAppend().appendFile(FILE_A).commit();
+ Snapshot mainAppend = table.currentSnapshot();
+
+ table.manageSnapshots().createBranch(branch,
mainAppend.snapshotId()).commit();
+
+ table.newRowDelta().addDeletes(FILE_A_DELETES).toBranch(branch).commit();
+ Snapshot branchDelta = table.snapshot(branch);
+ assertThat(branchDelta.deleteManifests(table.io()))
+ .as("Branch tip should own a delete manifest referencing
FILE_A_DELETES")
+ .hasSize(1);
+
+ // Remove the branch so its snapshots and manifests become expiration
candidates.
+ table.manageSnapshots().removeBranch(branch).commit();
Review Comment:
Yes- the branched setup is the most deterministic reproducer, not the only
affected scenario. Any invocation of
ExpireSnapshots.withIncrementalCleanup(false) on a table that has DELETE
manifests reachable only through expired snapshots would hit the same
IllegalArgumentException-then-swallow path. I kept the branch construct in the
regression test because it guarantees the DELETE manifest is uniquely owned by
an expired snapshot with no chance of a subsequent commit merging or promoting
it into the current table state, which keeps the assertion unambiguous. The fix
in ManifestFiles.readPaths covers both branched and non-branched cases.
--
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]
