dramaticlly commented on code in PR #17745:
URL: https://github.com/apache/iceberg/pull/17745#discussion_r3826598842
##########
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:
I think even in formatVersion 3 we still have delete manifest? So I would go
2 & 3 in this case as v4 is unified in root manifest
##########
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:
is the code path reachable in non-branched write of delete manifests?
##########
core/src/main/java/org/apache/iceberg/ReachableFileCleanup.java:
##########
@@ -183,9 +184,15 @@ private Set<String> findFilesToDelete(
"Failed to determine live files in manifest {}. Retrying",
item.path(), exc))
.run(
manifest -> {
- try (CloseableIterable<String> paths =
- ManifestFiles.readPaths(manifest, fileIO, specsById)) {
Review Comment:
curious if worth change logic directly in `ManifestFiles.readPaths`
directly? As I think this class is the only caller for
`ManifestFiles.readPaths` from
https://github.com/search?q=repo%3Aapache%2Ficeberg+ManifestFiles.readPaths&type=code
--
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]
