refactor: harden FileCleanupStrategy with retry and parallel deletes#649
Open
shangxinli wants to merge 11 commits intoapache:mainfrom
Open
refactor: harden FileCleanupStrategy with retry and parallel deletes#649shangxinli wants to merge 11 commits intoapache:mainfrom
shangxinli wants to merge 11 commits intoapache:mainfrom
Conversation
Implement the file cleanup logic that was missing from the expire
snapshots feature (the original PR noted "TODO: File recycling will
be added in a followup PR").
Port the "reachable file cleanup" strategy from Java's
ReachableFileCleanup, following the same phased approach:
Phase 1: Collect manifest paths from expired and retained snapshots
Phase 2: Prune manifests still referenced by retained snapshots
Phase 3: Find data files only in manifests being deleted, subtract
files still reachable from retained manifests (kAll only)
Phase 4: Delete orphaned manifest files
Phase 5: Delete manifest lists from expired snapshots
Phase 6: Delete expired statistics and partition statistics files
Key design decisions matching Java parity:
- Best-effort deletion: suppress errors on individual file deletions
to avoid blocking metadata updates (Java suppressFailureWhenFinished)
- Branch/tag awareness: retained snapshot set includes all snapshots
reachable from any ref (branch or tag), preventing false-positive
deletions of files still referenced by non-main branches
- Data file safety: only delete data files from manifests that are
themselves being deleted, then subtract any files still reachable
from retained manifests (two-pass approach from ReachableFileCleanup)
- Respect CleanupLevel: kNone skips all, kMetadataOnly skips data
files, kAll cleans everything
- FileIO abstraction: uses FileIO::DeleteFile for filesystem
compatibility (S3, HDFS, local), with custom DeleteWith() override
- Statistics cleanup via snapshot ID membership in retained set
TODOs for follow-up:
- Multi-threaded file deletion (Java uses Tasks.foreach with executor)
- IncrementalFileCleanup strategy for linear ancestry optimization
(Java uses this when no branches/cherry-picks involved)
- Fix O(M*S) I/O: Pre-cache ManifestFile objects in manifest_cache_ during Phase 1 (ReadManifestsForSnapshot), eliminating repeated manifest list reads in FindDataFilesToDelete. - Fix storage leak: Use LiveEntries() instead of Entries() to match Java's ManifestFiles.readPaths behavior (only ADDED/EXISTING entries). - Fix data loss risk: When reading a retained manifest fails, abort data file deletion entirely instead of silently continuing. Java retries and throws on failure here. - Fix statistics file deletion: Use path-based set difference instead of snapshot_id-only check, preventing erroneous deletion of statistics files shared across snapshots. - Remove goto anti-pattern: Extract ManifestFile lookup into MakeManifestReader() helper and use manifest_cache_ for direct lookup. - Improve API: FindDataFilesToDelete now returns Result<unordered_set<string>> instead of using a mutable out-parameter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Mirror Java's file cleanup class hierarchy for expire snapshots: - Add abstract FileCleanupStrategy with shared DeleteFile() and ExpiredStatisticsFilePaths() utilities (path-based set difference) - Add ReachableFileCleanup concrete class owning manifest_cache_, ReadManifestsForSnapshot(), and FindDataFilesToDelete() - Move MakeManifestReader() to a free function in anonymous namespace using ICEBERG_ASSIGN_OR_RAISE - Remove cleanup-specific private methods and manifest_cache_ from ExpireSnapshots class; Finalize() now delegates to the strategy - Clear apply_result_ after consumption in Finalize() - Rename DeleteFilePath to DeleteFile; use std::ignore for FileIO return - Remove manifest_list.h and manifest_reader.h from the header
… stats file deletion P0: ReadManifestsForSnapshot now returns bool. If any retained snapshot's manifest list cannot be read, phases 2-4 (manifest and data file deletion) are skipped entirely. An incomplete retained set makes it unsafe to compute manifests_to_delete, as manifests still referenced by unreadable snapshots would be wrongly included. This matches Java's throwFailureWhenFinished behavior in ReachableFileCleanup. Manifest list deletion (phase 5) is unaffected since it is keyed on expired snapshots only. P1: Remove physical statistics and partition-statistics file deletion (the former phase 6). RemoveStatistics/RemovePartitionStatistics are still not called in RemoveSnapshots (the TODO in table_metadata.cc), so the committed metadata still references those files after they would be deleted on disk. Deletion is deferred until the metadata-level removal is wired in, at which point the two operations can be kept in sync.
Brings the C++ cleanup path closer to Java's behavior: - DeleteFile retries up to 3 times on FileIO-backed errors with linear backoff, stopping immediately on kNotFound (mirrors Java's stopRetryOn(NotFoundException) + retry(3)). Custom delete callbacks remain single-shot since their retry policy is opaque to us. - DeleteFiles now parallelizes per-file deletes through a small std::async-based RunInParallel wrapper, capped at 8 workers to avoid swamping FileIO. Replaces the existing serial loop and resolves the TODO(shangxinli) marker on bulk deletion (a true bulk FileIO API remains TODO). - DeleteWith() doc note clarifies that the supplied callback may be invoked concurrently and must be thread-safe. No behavioral test changes are needed -- the existing cleanup tests exercise the parallel path automatically when they delete more than one file, and continue to pass.
The merge of main into pr-b-cleanup-hardening auto-resolved by keeping
both the new and old versions side-by-side in three places:
* transaction.cc: duplicate Result<const TableMetadata*>
finalize_result definition (compile error: redefinition).
* expire_snapshots.cc: duplicate #include <string>.
* expire_snapshots.cc: an old DeleteFile/DeleteFiles pair (the
pre-hardening serial loop with the original "TODO add retry"
comment) was kept inside the new DeleteFiles body, leaving an
unmatched brace and two redefined methods.
Drop the duplicates so the file compiles and matches the intended
post-merge state. clang-format remains clean and all ExpireSnapshots
tests pass.
The cleanup tests collected deleted paths via deleted_files.push_back inside the DeleteWith callback. With the new parallel DeleteFiles, that callback can now run concurrently from multiple worker threads, racing on a non-thread-safe std::vector. The race surfaced as a flaky failure in CI's ASAN/UBSAN job for ExpireSnapshotsCleanupTest.MetadataOnlySkipsDataDeletion: the test expected three deleted entries, but one was lost to the race. Local serial-run timing happened to mask it. Wrap each test's collector with a per-test std::mutex so the push_back is serialized. Functionally equivalent for sequential deletes; correct under parallel deletes. 20-iteration loop is now green locally.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
refactor: harden FileCleanupStrategy with retry and parallel deletes. Brings the C++ cleanup path closer to Java's behavior:
No behavioral test changes are needed -- the existing cleanup tests exercise the parallel path automatically when they delete more than one file, and continue to pass.