feat(core): introduce snapshot cleanup operations#57
Conversation
|
Thanks @chongchongxiao for your previous contributions to paimon-cpp. This migration PR carries that work forward into Apache Paimon C++. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR introduces a C++ orphan files cleaning API/implementation with metrics, along with snapshot expiration logic and accompanying unit tests.
Changes:
- Added
OrphanFilesCleanerpublic API (CleanContext+ builder) andOrphanFilesCleanerImplimplementation to detect and delete orphan files. - Added cleaning metrics (
CleanMetrics) and unit tests for the orphan cleaner behavior/unsupported-table cases. - Added
ExpireSnapshotsimplementation and tests for snapshot expiration and manifest/data-file deletion.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/paimon/core/operation/orphan_files_cleaner_test.cpp | New unit tests covering orphan cleaner patterns and unsupported table scenarios. |
| src/paimon/core/operation/orphan_files_cleaner_impl.h | Declares the orphan cleaner implementation (internal). |
| src/paimon/core/operation/orphan_files_cleaner_impl.cpp | Implements orphan file discovery/deletion and metrics emission. |
| src/paimon/core/operation/orphan_files_cleaner.cpp | Implements CleanContextBuilder and OrphanFilesCleaner::Create. |
| src/paimon/core/operation/metrics/clean_metrics.h | Adds metric keys for the orphan cleaning operation. |
| src/paimon/core/operation/expire_snapshots_test.cpp | New unit tests for snapshot expiration edge cases and data-file deletion mapping. |
| src/paimon/core/operation/expire_snapshots.h | Declares the snapshot expiration operation. |
| src/paimon/core/operation/expire_snapshots.cpp | Implements snapshot expiration, manifest cleanup, and empty-directory cleanup. |
| include/paimon/orphan_files_cleaner.h | Adds the public orphan files cleaner API and docs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| std::set<std::string> ListFileDirs(const std::string& path, int32_t max_level) const; | ||
| Result<std::set<std::string>> GetUsedFiles() const; | ||
| Result<std::set<std::string>> GetUsedFilesBySnapshot(const Snapshot& snapshot) const; | ||
| static bool SupportToClean(const std::string& file_name); |
| } | ||
| PAIMON_ASSIGN_OR_RAISE(std::set<std::string> used_file_names, GetUsedFiles()); | ||
|
|
||
| Duration duration; |
| uint64_t file_statuses_duration = duration.Reset(); | ||
| for (const auto& file_statuses : CollectAll(file_statuses_futures)) { |
| need_to_deletes.insert(path); | ||
| futures.push_back(Via(executor_.get(), [this, path]() { | ||
| auto s = fs_->Delete(path, /*recursive=*/false); | ||
| (void)s; | ||
| })); |
| for (const auto& empty_directory : to_delete_empty_directories) { | ||
| futures.push_back(Via(executor_.get(), [this, &empty_directory] { | ||
| auto ret = TryDeleteEmptyDirectory(empty_directory); | ||
| (void)ret; | ||
| })); | ||
| } |
| // TODO(jinli.zjw): skip index manifests | ||
| if (snapshot.IndexManifest() && snapshot.IndexManifest().value() != "") { | ||
| assert(false); | ||
| return Status::NotImplemented("do not support expire snapshot with index manifest"); | ||
| } |
| } | ||
| } | ||
|
|
||
| for (int32_t hierarchy = deduplicate.size() - 1; hierarchy >= 0; hierarchy--) { |
| if (max_deletes < 0) { | ||
| return Status::Invalid(fmt::format( | ||
| "Expire failed: snapshot max delete num '{}' must be greater than 0", max_deletes)); | ||
| } |
| std::vector<std::string> to_delete_manifests; | ||
| // TODO(jinli.zjw): optimize for async | ||
| for (const auto& manifest_file_meta : manifest_file_metas) { |
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
Thanks for migrating this feature. I think we need a couple of correctness fixes before merging:
-
snapshot.time-retainedis checked against the current candidate snapshot instead of its next snapshot. SnapshotNshould only be expired after snapshotN + 1has been retained long enough. With the current loop, if snapshot5is still within the retention window,ExpireUntil(..., 5)can still delete snapshot4, even though its successor is not old enough yet. Please align this with the Java implementation by checkingsnapshot_id + 1and add a regression test for the boundary. -
CleanEmptyDirectoriesschedules async deletion tasks that capture the range-for variable by reference ([this, &empty_directory]). Those tasks may execute after the loop variable has advanced or gone out of scope, so this is a use-after-scope risk and may delete the wrong directory. Please capture the path by value, e.g.[this, empty_directory], and add coverage if possible.
Purpose
Linked issue: close #xxx
Introduce snapshot expiration and orphan file cleanup operations from the Alibaba Paimon C++ repository.
Migrated files:
include/paimon/orphan_files_cleaner.hsrc/paimon/core/operation/expire_snapshots.hsrc/paimon/core/operation/expire_snapshots.cppsrc/paimon/core/operation/expire_snapshots_test.cppsrc/paimon/core/operation/orphan_files_cleaner.cppsrc/paimon/core/operation/orphan_files_cleaner_impl.hsrc/paimon/core/operation/orphan_files_cleaner_impl.cppsrc/paimon/core/operation/orphan_files_cleaner_test.cppsrc/paimon/core/operation/metrics/clean_metrics.hThe dependency closure is intentionally not included in this PR.
Tests
git diff --cached --checkgit diff --checkAPI and Format
This change adds the public
orphan_files_cleaner.hAPI. It does not change storage format or protocol.Documentation
No documentation changes.
Generative AI tooling
Migrated-by: OpenAI Codex