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:
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]
