dkranchii opened a new pull request, #17745:
URL: https://github.com/apache/iceberg/pull/17745
## Summary
`ReachableFileCleanup#findFilesToDelete` uses `ManifestFiles.readPaths` to
enumerate live file paths for every manifest scheduled for deletion.
`readPaths` is only defined for DATA manifests — it calls `ManifestFiles.read`
which asserts `manifest.content() == ManifestContent.DATA` and throws
`IllegalArgumentException` otherwise. Because the surrounding
`Tasks.foreach(...).retry(3).suppressFailureWhenFinished()` catches only
`IOException` explicitly and suppresses everything else, the exception is
logged per retry and swallowed, so DELETE manifests reachable only through
expired snapshots are silently skipped. The delete files and DV Puffin blobs
they reference are left as orphans on object storage.
`ReachableFileCleanup` is selected by `RemoveSnapshots.cleanExpirenapshots`
whenever specific snapshot IDs are given, when a non-main snapshot was removed,
or when there are non-main snapshots — i.e., any workflow that touches branches
or tags.
`IncrementalFileCleanup.findFilesToDelete` already handles this correctly by
using `ManifestFiles.open`, which returns a `ManifestReader` for either DATA or
DELETE manifest content. This PR aligns `ReachableFileCleanup` with that
pattern and adds a regression test.
## Change
- Replace both `ManifestFiles.readPaths(...)` calls in
`ReachableFileCleanup#findFilesToDelete` with
`ManifestFiles.open(...).select("file_path").liveEntries()`, projecting only
the file-path column so the read cost is unchanged.
- New test
`TestRemoveSnapshots#testReachableCleanupWithDeleteManifestFromRemovedBranch`
stages a position-delete file on a branch, removes the branch, expires older
snapshots, and asserts the delete-file location is enumerated for deletion.
Gated with `assumeThat(formatVersion == 2)` and
`assumeThat(!incrementalCleanup)` to target t reachable cleanup path.
## Test plan
- [ ] `./gradlew :iceberg-core:test --tests
"org.apache.iceberg.TestRemoveSnapshots"` passes.
- [ ] The new regression test fails without the code change and passes with
it.
- [ ] `./gradlew spotlessCheck` passes.
---
**AI Disclosure**
- Platform/Tool: Cursor
--
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]
