dkranchii commented on code in PR #17745:
URL: https://github.com/apache/iceberg/pull/17745#discussion_r3831875542
##########
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);
Review Comment:
@dramaticlly Good catch, thanks. Updated to
assumeThat(formatVersion).isIn(2, 3) - V1 has no delete files and V4+ unifies
delete entries into the root manifest, so V2 and V3 are the only versions with
standalone delete manifests. Note the class-level @Parameters still only cycles
v1/v2 today, so at runtime this behaves the same, but the assertion now
correctly documents the actual scope. Happy to extend the parameterization to
include v3 in a separate PR if that would be useful for the class more broadly.
--
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]
