Standalone purger support - #8
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the existing single object path purger with an improved standalone purger that supports file number thresholds. The main purpose is to enhance the cloud storage purger functionality with better file lifecycle management and threshold-based deletion logic.
Key changes:
- Replace
single_object_path_purge.ccwithimproved_purger.cccontaining enhanced purger logic - Add
GetCurrentEpochmethod to theDBCloudinterface and implementation - Update build configurations to use the new purger implementation
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src.mk | Updates source file reference from old to new purger implementation |
| include/rocksdb/cloud/db_cloud.h | Adds GetCurrentEpoch method declaration to DBCloud interface |
| cloud/improved_purger.cc | New comprehensive purger implementation with file number threshold support |
| cloud/db_cloud_impl.h | Adds GetCurrentEpoch method declaration to implementation class |
| cloud/db_cloud_impl.cc | Implements GetCurrentEpoch method and code formatting improvements |
| TARGETS | Updates build target to use new purger source file |
| CMakeLists.txt | Updates CMake configuration to use new purger source file |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@coderabbitai review |
1 similar comment
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces the previous single-object purger with a new EloqPurger implementation and CLI; adds DBCloud::GetCurrentEpoch and DBCloudImpl::GetCurrentEpoch; introduces per-epoch threshold-driven purge logic and S3 threshold readers; refines WarmUp checks/logging; updates build targets and sources across CMake, Buck TARGETS, Makefile, and src.mk. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as eloq_purger_command
participant CFS as CloudFileSystemImpl
participant Purger as EloqPurger
participant S3 as ObjectStore
participant CM as CloudManifest
User->>CLI: invoke (s3_url, dry_run, creds)
CLI->>CLI: ParseS3Url / init AWS (if used) / build options
CLI->>CFS: Create CloudFileSystem (options)
CLI->>Purger: Construct(cfs, bucket, object_path, dry_run)
CLI->>Purger: RunSinglePurgeCycle()
rect rgba(220,240,255,0.35)
Purger->>CFS: ListAllFiles(object_path)
CFS-->>Purger: object list
Purger->>CFS: ListCloudManifests()
CFS-->>Purger: manifest objects
Purger->>CM: Load manifests
CM-->>Purger: manifest data
Purger->>S3: Read epoch thresholds (per manifest)
S3-->>Purger: min file numbers
Purger->>Purger: Select obsolete (non-live & < threshold)
alt dry_run
Purger-->>CLI: Report would-delete list
else
Purger->>CFS: Delete obsolete objects
CFS->>S3: Delete operations
S3-->>CFS: Delete responses
end
end
Purger-->>CLI: Summary (counts, duration)
CLI-->>User: Exit / logs
sequenceDiagram
autonumber
participant Client
participant DB as DBCloudImpl
participant FS as CloudFileSystemImpl
participant CM as CloudManifest
Client->>DB: GetCurrentEpoch(epoch*)
DB->>FS: GetDbIdentity() / cast to CloudFileSystemImpl
alt is Cloud FS
DB->>CM: cfs->GetCloudManifest()->GetCurrentEpoch()
CM-->>DB: epoch string
DB-->>Client: Status::OK + epoch
else not Cloud FS
DB-->>Client: Status::NotSupported
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CMakeLists.txt (1)
979-985: Add/enable Eloq purger sources & CLI target in CMakeLists.txtcloud/eloq_purger.cc, cloud/eloq_purger.h and cloud/eloq_purger_command.cc exist in cloud/ but are not wired into CMakeLists.txt (improved_purger.cc is listed around line ~980). Apply these diffs:
- Add the Eloq purger implementation to the library:
# cloud/purge.cc - cloud/improved_purger.cc + cloud/improved_purger.cc + cloud/eloq_purger.cc cloud/cloud_manifest.cc
- Add a build toggle and the CLI target under the existing tools section:
@@ option(WITH_CORE_TOOLS "build with ldb and sst_dump" ON) option(WITH_TOOLS "build with tools" ON) +option(WITH_ELOQ_PURGER_TOOL "build eloq_purger CLI" ON) @@ if(WITH_TOOLS) add_subdirectory(db_stress_tool) + if(WITH_ELOQ_PURGER_TOOL) + add_executable(eloq_purger${ARTIFACT_SUFFIX} + cloud/eloq_purger_command.cc) + target_link_libraries(eloq_purger${ARTIFACT_SUFFIX} + ${ROCKSDB_LIB} ${GFLAGS_LIB} ${FOLLY_LIBS} ${THIRDPARTY_LIBS}) + list(APPEND tool_deps eloq_purger${ARTIFACT_SUFFIX}) + install(TARGETS eloq_purger${ARTIFACT_SUFFIX} + RUNTIME DESTINATION "${CMAKE_INSTALL_BINDIR}") + endif() add_custom_target(tools DEPENDS ${tool_deps}) endif()
🧹 Nitpick comments (19)
CMakeLists.txt (1)
979-981: Clean up stale commented source entry.
# cloud/purge.cclooks obsolete—remove it or add a brief note pointing to the new Eloq purger to avoid confusion.include/rocksdb/cloud/db_cloud.h (1)
70-72: Clarify API contract (format/thread-safety).Briefly document expected epoch format and whether it’s safe post-Close().
src.mk (1)
447-449: ELOQ_PURGER_SOURCES is defined but unused.Either remove it or append it to TOOLS_MAIN_SOURCES so it participates in dependency checks and header validation.
Apply one of the following:
Option A — wire it into tools:
ELOQ_PURGER_SOURCES = \ cloud/eloq_purger_command.cc \ +TOOLS_MAIN_SOURCES += $(ELOQ_PURGER_SOURCES)Option B — drop the dead var:
-ELOQ_PURGER_SOURCES = \ - cloud/eloq_purger_command.cc \ +# (removed unused ELOQ_PURGER_SOURCES)Makefile (2)
1341-1343: Make the CLI build with ‘make tools’ and link correct libs.Right now the binary isn’t part of the “tools” aggregate and only links librocksdb. If the command uses tools helpers, link TOOLS_LIBRARY and add it to the tools goal.
Apply this diff:
-eloq_purger_command: $(OBJ_DIR)/cloud/eloq_purger_command.o $(LIBRARY) +eloq_purger_command: $(OBJ_DIR)/cloud/eloq_purger_command.o $(TOOLS_LIBRARY) $(LIBRARY) $(AM_LINK) + +# Build it with the rest of the tools -tools: $(TOOLS) +tools: $(TOOLS) eloq_purger_command
1341-1343: Optional: add install/clean hooks.If distributed, consider adding it to ‘clean’ and an install target.
cloud/db_cloud_impl.cc (5)
248-266: Fix log message, remove unused dbid, and harden null checks.
- Message says “Savepoint” in GetCurrentEpoch.
- dbid is unused; avoid the GetDbIdentity call.
- Add a defensive check for a missing cloud manifest.
Apply this diff:
-Status DBCloudImpl::GetCurrentEpoch(std::string *epoch) const { - std::string dbid; - Options default_options = GetOptions(); - Status st = GetDbIdentity(dbid); - if (!st.ok()) { - Log(InfoLogLevel::INFO_LEVEL, default_options.info_log, - "Savepoint could not get dbid %s", st.ToString().c_str()); - return st; - } +Status DBCloudImpl::GetCurrentEpoch(std::string* epoch) const { + Options default_options = GetOptions(); auto* cfs = dynamic_cast<CloudFileSystemImpl*>(GetEnv()->GetFileSystem().get()); if (!cfs) { return Status::NotSupported( "GetCurrentEpoch is not supported for non-cloud file systems"); } - *epoch = cfs->GetCloudManifest()->GetCurrentEpoch(); + auto* manifest = cfs->GetCloudManifest(); + if (manifest == nullptr) { + return Status::NotFound("Cloud manifest not loaded"); + } + *epoch = manifest->GetCurrentEpoch(); return Status::OK(); }
335-354: Per‑shard cache check ties to LRUCache only.Good optimization, but using Name()=="LRUCache" + static_cast increases coupling. Consider a small interface on Cache for shard stats to avoid RTTI.
360-366: Reduce log spam in multi‑thread WarmUp.“start to fetch” logs once per thread. Log once before launching threads or include thread id.
387-401: Drop redundant assert after handling error.After the early-continue on !io_status.ok(), the subsequent assert(io_status.ok()) is redundant.
- assert(io_status.ok());
280-283: Consider NotSupported for non‑cloud fs in WarmUp too.You assert(cfs) today; returning NotSupported may be friendlier in integrated environments.
cloud/eloq_purger_command.cc (4)
176-178: Avoid abort() in library factory.
Return nullptr; let caller handle and surface an error.- std::cerr << "Invalid S3 endpoint url" << std::endl; - std::abort(); + std::cerr << "Invalid S3 endpoint url" << std::endl; + return nullptr;
86-95: Update ParseS3Url doc to include endpoint output.
Clarify behavior for http/https vs s3 URLs.- * @brief Parse URL into bucket and object path components - * @param url URL in format s3://bucket/path or - * http(s)://server:port/bucket/path - * @param bucket_name Output bucket name - * @param object_path Output object path + * @brief Parse S3/HTTP(S) URL into endpoint (optional), bucket and object path + * @param url URL in format s3://bucket/path or http(s)://server:port/bucket/path + * @param endpoint Output endpoint (only for http/https URLs; empty for s3://) + * @param bucket_name Output bucket name + * @param object_path Output object path
212-215: Align usage text with actual defaults and expose optional creds.
The default region is empty; list optional key flags for clarity.- std::cerr << " --aws_region=us-west-2 AWS region\n"; + std::cerr << " --aws_region=<region> AWS region (optional)\n"; + std::cerr << " --aws_access_key=<id> AWS Access Key ID (optional)\n"; + std::cerr << " --aws_secret_key=<secret> AWS Secret Access Key (optional)\n";
327-330: Avoid logging with a null logger in catch path.
Use stderr when logger is unavailable.- Log(rocksdb::InfoLogLevel::ERROR_LEVEL, nullptr, "Exception: %s", e.what()); - return 1; + std::cerr << "Exception: " << e.what() << std::endl; + return 1;cloud/eloq_purger.h (1)
46-61: Fix class/doc naming and sentinel semantics.
The class reads (not writes) and the comment says UINT64_MAX but impl uses UINT64_MIN as the sentinel.-/** - * @brief S3 file updater for writing smallest file number to S3 - */ +/** + * @brief S3 file reader for the smallest file number stored in S3 + */ @@ - /** - * @brief Read the smallest file number from S3 - * @return The smallest file number, or UINT64_MAX if not found - */ + /** + * @brief Read the smallest file number from S3. + * On missing/unreadable object, sets *file_number to UINT64_MIN (sentinel) and returns non-OK Status. + */ Status ReadSmallestFileNumber(uint64_t *file_number);cloud/eloq_purger.cc (4)
23-23: Remove unused gflags include.
This translation unit doesn’t use gflags.-#include <gflags/gflags.h>
71-88: Promote failures to WARN/ERROR and keep logger consistent.
Make severity match the condition; don’t log to nullptr.- if (!s.ok()) { - Log(InfoLogLevel::INFO_LEVEL, cfs_->info_log_, + if (!s.ok()) { + Log(InfoLogLevel::WARN_LEVEL, cfs_->info_log_, "Failed to read smallest file number from S3: %s, object_key: %s, " "returning UINT64_MIN", s.ToString().c_str(), object_key.c_str()); *file_number = std::numeric_limits<uint64_t>::min(); return s; } @@ - if (!temp_file.is_open()) { - Log(InfoLogLevel::INFO_LEVEL, cfs_->info_log_, + if (!temp_file.is_open()) { + Log(InfoLogLevel::ERROR_LEVEL, cfs_->info_log_, "Failed to open temp file for reading smallest file number: %s, " "object_key: %s, returning UINT64_MIN", temp_file_path.c_str(), object_key.c_str()); *file_number = std::numeric_limits<uint64_t>::min(); return Status::IOError("Failed to open temp file"); }
102-114: Avoid null logger and fix Status construction.
Use the real logger; construct Status without printf-style formatting.- Log(InfoLogLevel::INFO_LEVEL, nullptr, + Log(InfoLogLevel::INFO_LEVEL, cfs_->info_log_, "Read smallest file number from S3: %llu, object_key: %s", static_cast<unsigned long long>(*file_number), object_key.c_str()); return Status::OK(); } catch (const std::exception &e) { - Log(InfoLogLevel::INFO_LEVEL, nullptr, + Log(InfoLogLevel::INFO_LEVEL, cfs_->info_log_, "Failed to parse smallest file number from S3 content: '%s', " "returning UINT64_MIN", content.c_str()); *file_number = std::numeric_limits<uint64_t>::min(); - return Status::Corruption("Failed to parse smallest file number: %s", - e.what()); + return Status::Corruption(std::string("Failed to parse smallest file number: ") + e.what()); }
381-386: Remove assert on thresholds map; handle absent thresholds.
Asserts fire in debug builds for older epochs; the else-branch already logs conservatively.- assert(threshold_it != thresholds.end() && - "Thresholds should have been loaded for all epochs"); - if (threshold_it != thresholds.end()) { + if (threshold_it != thresholds.end()) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
CMakeLists.txt(1 hunks)Makefile(2 hunks)TARGETS(2 hunks)cloud/db_cloud_impl.cc(5 hunks)cloud/db_cloud_impl.h(1 hunks)cloud/eloq_purger.cc(1 hunks)cloud/eloq_purger.h(1 hunks)cloud/eloq_purger_command.cc(1 hunks)cloud/single_object_path_purge.cc(0 hunks)include/rocksdb/cloud/db_cloud.h(1 hunks)src.mk(2 hunks)
💤 Files with no reviewable changes (1)
- cloud/single_object_path_purge.cc
🔇 Additional comments (12)
TARGETS (1)
406-407: LGTM — mirrored into rocksdb_whole_archive_lib; manual verification needed for leftover single_object_path_purge references.Sandbox rg returned "No files were searched", so I couldn't confirm. Run locally:
rg -n --hidden --no-ignore-vcs -S 'single_object_path_purge(.cc)?|SingleObjectPathPurger' || true
Confirm there are no matches.cloud/db_cloud_impl.h (1)
36-37: Override added correctly.Signature and constness match the interface.
src.mk (3)
29-31: Library purger source inclusion looks good.New purger is compiled into the lib; consistent with the migration.
2630-2630: Nit: endif fix is fine.
29-31: Confirm old purger removal across build files.No matches for "single_object_path_purge" or "single_object_path_purge.cc" found; repo references the new eloq_purger (src.mk, Makefile, cloud/eloq_purger*.cc). Re-run a targeted grep across CMakeLists, BUCK files, Makefiles, and scripts (or run rg --debug) to be certain.
cloud/db_cloud_impl.cc (4)
12-15: Non‑lite include placement is correct.Including LRUCache only under !ROCKSDB_LITE avoids lite build bloat.
68-75: Destructor joins are safer now.Setting the flag then joining prevents runaway background warm‑ups.
296-304: Join previous runs before starting new ones — good.Prevents thread leaks across repeated WarmUp calls.
248-266: GetCurrentEpoch is pure-virtual; no internal subclasses missing the override.
include/rocksdb/cloud/db_cloud.h:71 declares GetCurrentEpoch(...) = 0; cloud/db_cloud_impl.h:36 provides the override; search found no other: public DBCloudsubclasses in the repo — external subclasses would still be required to implement this method.cloud/eloq_purger.cc (3)
468-489: No action required — purger stop predicate verified.
purger_is_running_ is initialized to true (cloud/cloud_file_system_impl.cc:40) and only set to false during shutdown under purger_lock_ with purger_cv_.notify_one (cloud/cloud_file_system_impl.cc:892–893); wait_for(..., [&]{ return !purger_is_running_; }) is correct.
368-372: ends_with/RemoveEpoch/GetEpoch — definitions present; no action required.
RemoveEpoch, GetEpoch, and the free-function ends_with are defined in cloud/filename.h (ends_with at line 78, RemoveEpoch at line 101, GetEpoch at line 113), so the usage in cloud/eloq_purger.cc is valid.
425-433: No change required — ListCloudObjects returns relative names; deletion join is correct.
S3/GCS list implementations strip the object_path prefix and push substr(prefix.size()) (returning names relative to the prefix); callers join object_path_ + "/" + name when deleting. See cloud/aws/aws_s3.cc and cloud/gcp/gcp_cs.cc (ListCloudObjects / ListCloudObjectsWithPrefix) and usage in cloud/purge.cc / cloud/eloq_purger.cc.
7e9fbec to
bc4ec4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cloud/eloq_purger.cc (1)
343-367: Don’t abort the purge if a single epoch’s threshold is missing; skip that epoch (no deletions).Gracefully handle NotFound from S3 and continue. This matches the “be conservative” approach and avoids using UINT64_MIN as a sentinel.
Apply this diff:
- uint64_t threshold; - Status s = s3_updater->ReadSmallestFileNumber(&threshold); - if (!s.ok()) { - Log(InfoLogLevel::ERROR_LEVEL, cfs_->info_log_, - "[pg] Failed to read file number threshold for epoch %s: %s", - epoch.c_str(), s.ToString().c_str()); - return s; - } + uint64_t threshold; + Status s = s3_updater->ReadSmallestFileNumber(&threshold); + if (!s.ok()) { + if (s.IsNotFound()) { + Log(InfoLogLevel::WARN_LEVEL, cfs_->info_log_, + "[pg] Threshold object not found for epoch %s; skipping this epoch", + epoch.c_str()); + continue; // Conservative: no deletions for this epoch + } + Log(InfoLogLevel::ERROR_LEVEL, cfs_->info_log_, + "[pg] Failed to read threshold for epoch %s: %s", + epoch.c_str(), s.ToString().c_str()); + return s; + }
🧹 Nitpick comments (5)
cloud/eloq_purger.cc (3)
391-419: Remove UINT64_MIN sentinel dependency in selection logic.After skipping epochs with missing thresholds, there’s no need for a MIN sentinel gate here.
Apply this diff:
- auto threshold_it = thresholds.find(candidate_epoch); - if (threshold_it != thresholds.end()) { - uint64_t threshold = threshold_it->second; - if (threshold != std::numeric_limits<uint64_t>::min()) { - // Extract file number from candidate file name - uint64_t file_number = 0; - std::string base_name = RemoveEpoch(candidate_file_path); - FileType type; - if (ParseFileName(base_name, &file_number, &type)) { - if (file_number < threshold) { - obsolete_files->push_back(candidate_file_path); - Log(InfoLogLevel::INFO_LEVEL, cfs_->info_log_, - "[pg] File %s selected for deletion (file_num=%llu, " - "threshold=%llu)", - candidate_file_path.c_str(), - static_cast<unsigned long long>(file_number), - static_cast<unsigned long long>(threshold)); - } else { - Log(InfoLogLevel::INFO_LEVEL, cfs_->info_log_, - "[pg] Skipping obsolete file %s due to file number " - "threshold (file_num=%llu, threshold=%llu)", - candidate_file_path.c_str(), - static_cast<unsigned long long>(file_number), - static_cast<unsigned long long>(threshold)); - } - } - } - } else { + auto threshold_it = thresholds.find(candidate_epoch); + if (threshold_it != thresholds.end()) { + // Extract file number from candidate file name + uint64_t file_number = 0; + std::string base_name = RemoveEpoch(candidate_file_path); + FileType type; + if (ParseFileName(base_name, &file_number, &type)) { + uint64_t threshold = threshold_it->second; + if (file_number < threshold) { + obsolete_files->push_back(candidate_file_path); + Log(InfoLogLevel::INFO_LEVEL, cfs_->info_log_, + "[pg] File %s selected for deletion (file_num=%llu, threshold=%llu)", + candidate_file_path.c_str(), + static_cast<unsigned long long>(file_number), + static_cast<unsigned long long>(threshold)); + } else { + Log(InfoLogLevel::INFO_LEVEL, cfs_->info_log_, + "[pg] Skipping obsolete file %s due to file number threshold (file_num=%llu, threshold=%llu)", + candidate_file_path.c_str(), + static_cast<unsigned long long>(file_number), + static_cast<unsigned long long>(threshold)); + } + } + } else { Log(InfoLogLevel::INFO_LEVEL, cfs_->info_log_, "[pg] No threshold for epoch %s, using conservative approach - " "not deleting file %s", candidate_epoch.c_str(), candidate_file_path.c_str()); }
25-38: Include for errno usage.
strerror(errno)requires . Some platforms won’t provideerrnovia transitive includes.Apply this diff:
#include <chrono> +#include <cerrno> #include <cassert> #include <cstdio> #include <fstream>
360-424: Reduce per-file INFO logs or gate them behind verbosity.Per‑file INFO logs during selection can be noisy at scale; prefer DEBUG for skip/decision logs or add a verbosity flag.
cloud/db_cloud_impl.cc (2)
248-256: Fix log message context.Message says “Savepoint” in GetCurrentEpoch; use the correct context for easier ops debugging.
Apply this diff:
- if (!st.ok()) { - Log(InfoLogLevel::INFO_LEVEL, default_options.info_log, - "Savepoint could not get dbid %s", st.ToString().c_str()); + if (!st.ok()) { + Log(InfoLogLevel::INFO_LEVEL, default_options.info_log, + "GetCurrentEpoch: could not get dbid %s", st.ToString().c_str()); return st; }
257-266: Null‑check CloudManifest before dereference.Avoid potential NPEs if the manifest isn’t loaded yet.
Apply this diff:
- *epoch = cfs->GetCloudManifest()->GetCurrentEpoch(); + auto* cm = cfs->GetCloudManifest(); + if (cm == nullptr) { + return Status::NotSupported("GetCurrentEpoch: CloudManifest not loaded"); + } + *epoch = cm->GetCurrentEpoch();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CMakeLists.txt(1 hunks)Makefile(2 hunks)TARGETS(2 hunks)cloud/db_cloud_impl.cc(5 hunks)cloud/db_cloud_impl.h(1 hunks)cloud/eloq_purger.cc(1 hunks)cloud/eloq_purger.h(1 hunks)cloud/eloq_purger_command.cc(1 hunks)include/rocksdb/cloud/db_cloud.h(1 hunks)src.mk(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- cloud/db_cloud_impl.h
- CMakeLists.txt
- TARGETS
- Makefile
- src.mk
- cloud/eloq_purger.h
- cloud/eloq_purger_command.cc
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-17T07:23:50.943Z
Learnt from: githubzilla
PR: eloqdata/rocksdb-cloud#8
File: cloud/eloq_purger.cc:343-356
Timestamp: 2025-09-17T07:23:50.943Z
Learning: When handling missing file number thresholds for epoch-based purging, using UINT64_MIN (0) as a default is unsafe because it could cause excessive file deletion depending on the comparison logic. A safer approach is to use UINT64_MAX or skip the epoch entirely to be conservative.
Applied to files:
cloud/eloq_purger.cc
🧬 Code graph analysis (1)
cloud/eloq_purger.cc (3)
cloud/eloq_purger.h (2)
S3FileNumberReader(48-69)EloqPurger(74-127)cloud/filename.h (1)
ends_with(78-81)cloud/eloq_purger_command.cc (1)
purger(325-326)
🔇 Additional comments (3)
include/rocksdb/cloud/db_cloud.h (1)
70-74: Default non‑pure virtual is the right call.Prevents ABI/source breaks for external implementers. LGTM.
cloud/db_cloud_impl.cc (1)
280-354: WarmUp concurrency/cache gating changes look good.Shard‑aware cache usage and thread lifecycle handling are sound. No blocking issues spotted.
Also applies to: 360-416
cloud/eloq_purger.cc (1)
214-228: ```shell
#!/bin/bash
set -euo pipefailecho "PWD: $(pwd)"
echo
echo "Searching for literal 'return s;' in common C/C++ source files..."
rg -n -C2 --hidden --no-ignore-vcs --glob '!/third_party/' --glob '**/.{cc,cpp,c,h,hpp,h}' $'return\s+s\s;\s*$' || trueecho
echo "Searching for IOStatus definitions/uses and conversion helpers..."
rg -n --hidden --no-ignore-vcs --glob '!/third_party/' 'class\s+IOStatus|struct\s+IOStatus|IOStatus|status_to_io_status|io_status_to_status|ToStatus(|operator\s+Status\b' || trueecho
echo "List files that mention IOStatus (first 200 lines):"
rg -n --hidden --no-ignore-vcs --glob '!/third_party/' 'IOStatus' | sed -n '1,200p' || trueecho
echo "Search for ListCloudObjects occurrences:"
rg -n --hidden --no-ignore-vcs --glob '!/third_party/' 'ListCloudObjects' || trueecho
echo "Show cloud/eloq_purger.cc (if present) up to 400 lines:"
if [ -f cloud/eloq_purger.cc ]; then
sed -n '1,400p' cloud/eloq_purger.cc || true
else
echo "cloud/eloq_purger.cc not found"
fiecho
echo "Quick heuristic: find functions declared to return 'Status' that also mention 'IOStatus' in the same file (prints the surrounding matches):"
for f in $(rg -l --hidden --no-ignore-vcs --glob '!/third_party/' 'IOStatus' || true); do
echo "---- $f ----"
rg -n -C3 --hidden --no-ignore-vcs 'Status\s+[A-Za-z0-9_:<>]+\s*([^)])\s{' "$f" || true
rg -n -C3 --hidden --no-ignore-vcs 'IOStatus' "$f" || true
done</blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
cloud/eloq_purger.cc (5)
81-87: Map IOStatus→Status, handle NotFound, and clean up temp file; avoid sentinel writes.
- Return
Status::NotFoundwhen the S3 object is missing so caller can skip the epoch.- Convert other IO failures to
Status::IOError.- Remove the temp file before returning.
- Fix log message formatting (trailing comma).
- if (!s.ok()) { - Log(InfoLogLevel::ERROR_LEVEL, cfs_->info_log_, - "Failed to read smallest file number from S3: %s, object_key: %s, ", - s.ToString().c_str(), object_key.c_str()); - *file_number = std::numeric_limits<uint64_t>::min(); - return s; - } + if (!s.ok()) { + if (s.IsNotFound()) { + Log(InfoLogLevel::WARN_LEVEL, cfs_->info_log_, + "Threshold object not found: %s (object_key=%s)", + s.ToString().c_str(), object_key.c_str()); + std::remove(temp_file_path.c_str()); + return Status::NotFound("Threshold object not found"); + } + Log(InfoLogLevel::ERROR_LEVEL, cfs_->info_log_, + "Failed to read smallest file number from S3: %s (object_key=%s)", + s.ToString().c_str(), object_key.c_str()); + std::remove(temp_file_path.c_str()); + return Status::IOError(s.ToString()); + }
110-125: Use the actual logger and don’t assign UINT64_MIN on parse errors.Use
cfs_->info_log_(notnullptr). Return a structured error without modifying*file_number.try { *file_number = std::stoull(content); - Log(InfoLogLevel::INFO_LEVEL, nullptr, + Log(InfoLogLevel::INFO_LEVEL, cfs_->info_log_, "Read smallest file number from S3: %llu, object_key: %s", static_cast<unsigned long long>(*file_number), object_key.c_str()); return Status::OK(); } catch (const std::exception &e) { - Log(InfoLogLevel::ERROR_LEVEL, nullptr, - "Failed to parse smallest file number from S3 content: '%s', " - "returning UINT64_MIN", - content.c_str()); - *file_number = std::numeric_limits<uint64_t>::min(); + Log(InfoLogLevel::WARN_LEVEL, cfs_->info_log_, + "Failed to parse smallest file number from S3 content: '%s'", + content.c_str()); return Status::Corruption("Failed to parse smallest file number: %s", e.what()); }
352-360: Don’t abort the purge on a missing threshold; skip the epoch (or use UINT64_MAX).Treat
NotFoundas “no threshold available” and continue. This avoids over‑deletion risk and prevents spurious cycle aborts.uint64_t threshold; Status s = s3_updater->ReadSmallestFileNumber(&threshold); - if (!s.ok()) { - Log(InfoLogLevel::ERROR_LEVEL, cfs_->info_log_, - "[pg] Failed to read file number threshold for epoch %s: %s", - epoch.c_str(), s.ToString().c_str()); - return s; - } + if (!s.ok()) { + if (s.IsNotFound()) { + Log(InfoLogLevel::WARN_LEVEL, cfs_->info_log_, + "[pg] No threshold for epoch %s; skipping this epoch.", epoch.c_str()); + continue; // do not insert into the map + } + Log(InfoLogLevel::ERROR_LEVEL, cfs_->info_log_, + "[pg] Failed to read file number threshold for epoch %s: %s", + epoch.c_str(), s.ToString().c_str()); + return s; + }
90-99: Avoid sentinel writes on temp-file open failure.Just return the error; don’t mutate the out-param.
std::ifstream temp_file(temp_file_path); if (!temp_file.is_open()) { Log(InfoLogLevel::ERROR_LEVEL, cfs_->info_log_, "Failed to open temp file for reading smallest file number: %s, " "object_key: %s", temp_file_path.c_str(), object_key.c_str()); - *file_number = std::numeric_limits<uint64_t>::min(); return Status::IOError("Failed to open temp file"); }
64-76: Don’t write sentinel to out-param on mkstemp() failure.Leave
*file_numberuntouched on errors; just return the error.int fd = mkstemp(tmp_template); if (fd == -1) { Log(InfoLogLevel::ERROR_LEVEL, cfs_->info_log_, "Failed to create temp file for reading smallest file number from S3: " "%s, object_key: %s", strerror(errno), object_key.c_str()); - *file_number = std::numeric_limits<uint64_t>::min(); return Status::IOError("Failed to create temp file"); }
🧹 Nitpick comments (3)
cloud/eloq_purger.cc (3)
23-25: Add missing headers for strerror/errno; drop unused include.
strerror(errno)needs<cstring>and<cerrno>. Also,gflags/gflags.his unused in this TU.-#include <gflags/gflags.h> #include <unistd.h> #include <cassert> #include <chrono> #include <cstdio> +#include <cstring> // strerror +#include <cerrno> // errno #include <fstream> #include <limits> #include <memory> #include <set> #include <sstream> #include <string> #include <unordered_map> #include <utility> #include <vector>Also applies to: 26-37
464-468: Lower severity: prerequisites not met is not an error.Use INFO/WARN instead of ERROR to avoid noisy logs in expected configurations.
- Log(InfoLogLevel::ERROR_LEVEL, cfs.info_log_, + Log(InfoLogLevel::INFO_LEVEL, cfs.info_log_, "[pg] Single Object Path Purger is not running because the " "prerequisites are not met.");
192-201: Reduce INFO-level noise for large deletions.Per-file INFO logs in dry-run and delete loops can flood logs at scale; consider DEBUG for per-file lines, keep a compact INFO summary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cloud/eloq_purger.cc(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-17T07:23:50.970Z
Learnt from: githubzilla
PR: eloqdata/rocksdb-cloud#8
File: cloud/eloq_purger.cc:343-356
Timestamp: 2025-09-17T07:23:50.970Z
Learning: When handling missing file number thresholds for epoch-based purging, using UINT64_MIN (0) as a default is unsafe because it could cause excessive file deletion depending on the comparison logic. A safer approach is to use UINT64_MAX or skip the epoch entirely to be conservative.
Applied to files:
cloud/eloq_purger.cc
🧬 Code graph analysis (1)
cloud/eloq_purger.cc (3)
cloud/eloq_purger.h (2)
S3FileNumberReader(48-69)EloqPurger(74-127)cloud/filename.h (1)
ends_with(78-81)cloud/eloq_purger_command.cc (1)
purger(325-326)
| if (threshold != std::numeric_limits<uint64_t>::min()) { | ||
| // Extract file number from candidate file name | ||
| uint64_t file_number = 0; | ||
| std::string base_name = RemoveEpoch(candidate_file_path); | ||
| FileType type; | ||
| if (ParseFileName(base_name, &file_number, &type)) { | ||
| if (file_number < threshold) { | ||
| obsolete_files->push_back(candidate_file_path); | ||
| Log(InfoLogLevel::INFO_LEVEL, cfs_->info_log_, | ||
| "[pg] File %s selected for deletion (file_num=%llu, " | ||
| "threshold=%llu)", | ||
| candidate_file_path.c_str(), | ||
| static_cast<unsigned long long>(file_number), | ||
| static_cast<unsigned long long>(threshold)); | ||
| } else { | ||
| Log(InfoLogLevel::INFO_LEVEL, cfs_->info_log_, | ||
| "[pg] Skipping obsolete file %s due to file number " | ||
| "threshold (file_num=%llu, threshold=%llu)", | ||
| candidate_file_path.c_str(), | ||
| static_cast<unsigned long long>(file_number), | ||
| static_cast<unsigned long long>(threshold)); | ||
| } | ||
| } | ||
| } | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use UINT64_MAX as the “no-threshold” sentinel (or rely on missing entry).
Current code checks against UINT64_MIN; prefer UINT64_MAX if a sentinel is used, which is safer for a file_number < threshold comparison. Given the loader now skips missing epochs, this branch mainly guards future callers.
- uint64_t threshold = threshold_it->second;
- if (threshold != std::numeric_limits<uint64_t>::min()) {
+ uint64_t threshold = threshold_it->second;
+ if (threshold != std::numeric_limits<uint64_t>::max()) {
// Extract file number from candidate file name📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (threshold != std::numeric_limits<uint64_t>::min()) { | |
| // Extract file number from candidate file name | |
| uint64_t file_number = 0; | |
| std::string base_name = RemoveEpoch(candidate_file_path); | |
| FileType type; | |
| if (ParseFileName(base_name, &file_number, &type)) { | |
| if (file_number < threshold) { | |
| obsolete_files->push_back(candidate_file_path); | |
| Log(InfoLogLevel::INFO_LEVEL, cfs_->info_log_, | |
| "[pg] File %s selected for deletion (file_num=%llu, " | |
| "threshold=%llu)", | |
| candidate_file_path.c_str(), | |
| static_cast<unsigned long long>(file_number), | |
| static_cast<unsigned long long>(threshold)); | |
| } else { | |
| Log(InfoLogLevel::INFO_LEVEL, cfs_->info_log_, | |
| "[pg] Skipping obsolete file %s due to file number " | |
| "threshold (file_num=%llu, threshold=%llu)", | |
| candidate_file_path.c_str(), | |
| static_cast<unsigned long long>(file_number), | |
| static_cast<unsigned long long>(threshold)); | |
| } | |
| } | |
| } | |
| } else { | |
| uint64_t threshold = threshold_it->second; | |
| if (threshold != std::numeric_limits<uint64_t>::max()) { | |
| // Extract file number from candidate file name | |
| uint64_t file_number = 0; | |
| std::string base_name = RemoveEpoch(candidate_file_path); | |
| FileType type; | |
| if (ParseFileName(base_name, &file_number, &type)) { | |
| if (file_number < threshold) { | |
| obsolete_files->push_back(candidate_file_path); | |
| Log(InfoLogLevel::INFO_LEVEL, cfs_->info_log_, | |
| "[pg] File %s selected for deletion (file_num=%llu, " | |
| "threshold=%llu)", | |
| candidate_file_path.c_str(), | |
| static_cast<unsigned long long>(file_number), | |
| static_cast<unsigned long long>(threshold)); | |
| } else { | |
| Log(InfoLogLevel::INFO_LEVEL, cfs_->info_log_, | |
| "[pg] Skipping obsolete file %s due to file number " | |
| "threshold (file_num=%llu, threshold=%llu)", | |
| candidate_file_path.c_str(), | |
| static_cast<unsigned long long>(file_number), | |
| static_cast<unsigned long long>(threshold)); | |
| } | |
| } | |
| } | |
| } else { |
🤖 Prompt for AI Agents
In cloud/eloq_purger.cc around lines 394 to 418, the code treats
std::numeric_limits<uint64_t>::min() as the "no-threshold" sentinel which is
incorrect for comparisons of the form file_number < threshold; change the
sentinel check to std::numeric_limits<uint64_t>::max() (or UINT64_MAX) so the
branch is only taken when a real threshold is present, i.e., replace the current
if (threshold != std::numeric_limits<uint64_t>::min()) condition with if
(threshold != std::numeric_limits<uint64_t>::max()) (or UINT64_MAX) and ensure
any callers or comments reflect that max is the sentinel for "no threshold".
There was a problem hiding this comment.
when threshold is zero, it actually mean the purger is blocked intentionally.
There was a problem hiding this comment.
@githubzilla Thank you for the important clarification! I understand now that a threshold value of zero (UINT64_MIN) is intentionally used as a safety mechanism to block purging, rather than being an error condition.
This means the current logic if (threshold != std::numeric_limits<uint64_t>::min()) is actually correct - when the threshold is zero, the purger is intentionally prevented from deleting files since no file number would be less than zero. This provides a deliberate safety control mechanism.
I'll update my understanding of this design pattern.
🐰
^.^
(o o)
_/
✏️ Learnings added
Learnt from: githubzilla
PR: eloqdata/rocksdb-cloud#8
File: cloud/eloq_purger.cc:394-418
Timestamp: 2025-09-17T09:48:20.172Z
Learning: In the EloqPurger implementation in cloud/eloq_purger.cc, when the file number threshold is zero (UINT64_MIN), it intentionally blocks the purger as a safety mechanism. The comparison `file_number < threshold` when threshold is 0 will never be true for valid file numbers, effectively preventing any file deletion. This is by design, not an error condition.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
⛔ Learnings removed
Learnt from: githubzilla
PR: eloqdata/rocksdb-cloud#8
File: cloud/eloq_purger.cc:343-356
Timestamp: 2025-09-17T07:23:50.970Z
Learning: When handling missing file number thresholds for epoch-based purging, using UINT64_MIN (0) as a default is unsafe because it could cause excessive file deletion depending on the comparison logic. A safer approach is to use UINT64_MAX or skip the epoch entirely to be conservative.
🧠 Learnings used
Learnt from: githubzilla
PR: eloqdata/rocksdb-cloud#8
File: cloud/eloq_purger.cc:343-356
Timestamp: 2025-09-17T07:23:50.970Z
Learning: When handling missing file number thresholds for epoch-based purging, using UINT64_MIN (0) as a default is unsafe because it could cause excessive file deletion depending on the comparison logic. A safer approach is to use UINT64_MAX or skip the epoch entirely to be conservative.
| uint64_t file_number = 0; | ||
| std::string base_name = RemoveEpoch(candidate_file_path); | ||
| FileType type; | ||
| if (ParseFileName(base_name, &file_number, &type)) { | ||
| if (file_number < threshold) { | ||
| obsolete_files->push_back(candidate_file_path); | ||
| Log(InfoLogLevel::INFO_LEVEL, cfs_->info_log_, | ||
| "[pg] File %s selected for deletion (file_num=%llu, " | ||
| "threshold=%llu)", |
There was a problem hiding this comment.
🧩 Analysis chain
Verify ParseFileName is fed a basename, not a path.
Depending on RocksDB version, ParseFileName may expect a basename. If RemoveEpoch(candidate_file_path) can still contain directories, extract the basename before parsing.
🏁 Script executed:
#!/bin/bash
# Inspect typical usage of ParseFileName across the codebase to confirm expectations.
rg -nP -C3 '\bParseFileName\s*\(' --type=cc --type=cppLength of output: 26
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== rg ParseFileName occurrences ==="
rg -n -S 'ParseFileName' || true
echo
echo "=== rg RemoveEpoch occurrences ==="
rg -n -S 'RemoveEpoch' || true
echo
echo "=== Show cloud/eloq_purger.cc (lines 340-410) ==="
if [ -f cloud/eloq_purger.cc ]; then
sed -n '340,410p' cloud/eloq_purger.cc || true
else
echo "cloud/eloq_purger.cc not found"
fi
echo
echo "=== Search for candidate_file_path usages ==="
rg -n -S 'candidate_file_path' || true
echo
echo "=== Attempt to find ParseFileName definition (likely in vendor/rocksdb) ==="
rg -n -S '^\s*(?:bool|int|static)\s+ParseFileName\s*\(|ParseFileName\s*\(\s*const\s+std::string' --hidden || trueLength of output: 16931
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== cloud/filename.h (full) ==="
if [ -f cloud/filename.h ]; then
sed -n '1,240p' cloud/filename.h || true
else
echo "cloud/filename.h not found"
fi
echo
echo "=== file/filename.h (full) ==="
if [ -f file/filename.h ]; then
sed -n '1,240p' file/filename.h || true
else
echo "file/filename.h not found"
fi
echo
echo "=== file/filename.cc (ParseFileName impl) ==="
if [ -f file/filename.cc ]; then
rg -n 'bool ParseFileName' file/filename.cc || true
lineno=$(rg -n 'bool ParseFileName' file/filename.cc | head -n1 | cut -d: -f1 || true)
if [ -n "$lineno" ]; then
start=$((lineno - 5))
if [ $start -lt 1 ]; then start=1; fi
end=$((lineno + 240))
sed -n "${start},${end}p" file/filename.cc || true
else
sed -n '1,400p' file/filename.cc || true
fi
else
echo "file/filename.cc not found"
fi
echo
echo "=== cloud/eloq_purger.cc (around usage) ==="
if [ -f cloud/eloq_purger.cc ]; then
rg -n 'RemoveEpoch' cloud/eloq_purger.cc || true
sed -n '360,420p' cloud/eloq_purger.cc || true
else
echo "cloud/eloq_purger.cc not found"
fiLength of output: 24743
Pass a basename to ParseFileName (not a path)
RemoveEpoch(candidate_file_path) can still contain directory components; ParseFileName (file/filename.cc) only strips a leading '/' and will fail on prefixed paths. In cloud/eloq_purger.cc (around lines 396–404) use basename(RemoveEpoch(candidate_file_path)) before calling ParseFileName.
🤖 Prompt for AI Agents
In cloud/eloq_purger.cc around lines 396 to 404,
RemoveEpoch(candidate_file_path) may still include directory components so
passing it directly to ParseFileName can fail; call basename() (or equivalent to
extract the filename only) on the result of RemoveEpoch(candidate_file_path) and
pass that basename string into ParseFileName. Ensure the basename result is
stored in a std::string (or a char* converted to std::string if using basename
from <libgen.h>) and use that variable when calling ParseFileName so only the
filename (no path) is parsed.
There was a problem hiding this comment.
I don't think this is matter. In this case, I just want to check if path end with .sst, I don't care if it returns a file name rather than a path. And the RemoveEpoch() function can work with path.
There was a problem hiding this comment.
@githubzilla, I understand your point. You're right that for your specific use case where you're primarily checking .sst extensions and RemoveEpoch() can handle paths, the current implementation should work for your purposes.
The main risk would be if ParseFileName fails to extract the file_number correctly from a path (rather than just a filename), which could affect the threshold comparison logic. But if you've tested this flow and it's working as expected, then you're good to go.
Thanks for the clarification!
🧠 Learnings used
Learnt from: githubzilla
PR: eloqdata/rocksdb-cloud#8
File: cloud/eloq_purger.cc:394-418
Timestamp: 2025-09-17T09:48:20.172Z
Learning: In the EloqPurger implementation in cloud/eloq_purger.cc, when the file number threshold is zero (UINT64_MIN), it intentionally blocks the purger as a safety mechanism. The comparison `file_number < threshold` when threshold is 0 will never be true for valid file numbers, effectively preventing any file deletion. This is by design, not an error condition.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
cloud/eloq_purger.cc (1)
353-361: Don’t abort the cycle on NotFound; treat as “blocked” (threshold=0) and continue.Matches the documented safety behavior: a 0 threshold intentionally blocks purging for that epoch.
uint64_t threshold; Status s = s3_updater->ReadSmallestFileNumber(&threshold); if (!s.ok()) { - Log(InfoLogLevel::ERROR_LEVEL, cfs_->info_log_, - "[pg] Failed to read file number threshold for epoch %s: %s", - epoch.c_str(), s.ToString().c_str()); - return s; + if (s.IsNotFound()) { + Log(InfoLogLevel::INFO_LEVEL, cfs_->info_log_, + "[pg] No threshold object for epoch %s; setting threshold=0 (blocked).", + epoch.c_str()); + threshold = std::numeric_limits<uint64_t>::min(); // 0 blocks purge + } else { + Log(InfoLogLevel::ERROR_LEVEL, cfs_->info_log_, + "[pg] Failed to read threshold for epoch %s: %s", + epoch.c_str(), s.ToString().c_str()); + return s; + } } (*thresholds)[epoch] = threshold;
🧹 Nitpick comments (2)
cloud/eloq_purger.cc (2)
83-84: Remove stray comma in log format string.Minor polish; avoids odd spacing in logs.
- "Failed to read smallest file number from S3: %s, object_key: %s, ", + "Failed to read smallest file number from S3: %s, object_key: %s",
420-424: Fix misleading log when threshold entry is missing.This branch is “no threshold loaded,” not “threshold is 0.”
- Log(InfoLogLevel::INFO_LEVEL, cfs_->info_log_, - "[pg] threshold is 0 for epoch %s, purge is blocked intentionally. " - "%s is skipped.", - candidate_epoch.c_str(), candidate_file_path.c_str()); + Log(InfoLogLevel::INFO_LEVEL, cfs_->info_log_, + "[pg] No threshold found for epoch %s; purging is blocked by default. " + "Skipping %s.", + candidate_epoch.c_str(), candidate_file_path.c_str());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cloud/eloq_purger.cc(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-17T09:48:20.191Z
Learnt from: githubzilla
PR: eloqdata/rocksdb-cloud#8
File: cloud/eloq_purger.cc:394-418
Timestamp: 2025-09-17T09:48:20.191Z
Learning: In the EloqPurger implementation in cloud/eloq_purger.cc, when the file number threshold is zero (UINT64_MIN), it intentionally blocks the purger as a safety mechanism. The comparison `file_number < threshold` when threshold is 0 will never be true for valid file numbers, effectively preventing any file deletion. This is by design, not an error condition.
Applied to files:
cloud/eloq_purger.cc
🧬 Code graph analysis (1)
cloud/eloq_purger.cc (3)
cloud/eloq_purger.h (2)
S3FileNumberReader(48-69)EloqPurger(74-127)cloud/filename.h (1)
ends_with(78-81)cloud/eloq_purger_command.cc (1)
purger(325-326)
🔇 Additional comments (2)
cloud/eloq_purger.cc (2)
459-471: Double-check prerequisite logic and message.Condition currently logs “not running…prerequisites are not met” when both src/dest are valid and different. Confirm this is the intended “block” condition for the standalone purger.
384-387: Confirm path shapes match; no double-prefix in observed flows
- ListCloudObjects (S3/GCS) returns names relative to the provided object_path prefix (it strips the prefix and pushes the remainder into result).
- RemapFilenameWithCloudManifest produces filenames without the bucket prefix (it uses basename(dirname) + filename + optional "-epoch"), matching the relative form returned by ListCloudObjects.
- DeleteObsoleteFiles constructs object_path_ + "/" + file_to_delete, which matches how EmptyBucket and other deletion call-sites build keys (they pass full key to DeleteCloudObject). S3 listing trims leading '/', but DeleteCloudObject does not — callers consistently pass object_path_ without a leading '/' in current flows.
Action: No change required here; the original concern is resolved by existing normalization and call patterns. Recommend ensuring object_path_ never contains a leading '/' at source (CloudFileSystem options) to avoid a mismatch on S3 delete calls.
Locations: cloud/eloq_purger.cc (SelectObsoleteFilesWithThreshold, DeleteObsoleteFiles), cloud/aws/aws_s3.cc (ListCloudObjects, DeleteCloudObject), cloud/gcp/gcp_cs.cc (ListCloudObjects, DeleteCloudObject), cloud/cloud_file_system_impl.cc (RemapFilenameWithCloudManifest).
| Status S3FileNumberReader::ReadSmallestFileNumber(uint64_t *file_number) { | ||
| std::string object_key = GetS3ObjectKey(); | ||
|
|
||
| // Write to temp local file at first | ||
| char tmp_template[] = | ||
| "/tmp/smallest_file_number_download_XXXXXX"; // Xs will be replaced | ||
| int fd = mkstemp(tmp_template); | ||
| if (fd == -1) { | ||
| Log(InfoLogLevel::ERROR_LEVEL, cfs_->info_log_, | ||
| "Failed to create temp file for reading smallest file number from S3: " | ||
| "%s, object_key: %s", | ||
| strerror(errno), object_key.c_str()); | ||
| *file_number = std::numeric_limits<uint64_t>::min(); | ||
| return Status::IOError("Failed to create temp file"); | ||
| } | ||
| close(fd); // We will open it later for reading | ||
| std::string temp_file_path = tmp_template; | ||
|
|
||
| rocksdb::IOStatus s = cfs_->GetStorageProvider()->GetCloudObject( | ||
| bucket_name_, object_key, temp_file_path); | ||
|
|
||
| if (!s.ok()) { | ||
| Log(InfoLogLevel::ERROR_LEVEL, cfs_->info_log_, | ||
| "Failed to read smallest file number from S3: %s, object_key: %s, ", | ||
| s.ToString().c_str(), object_key.c_str()); | ||
| *file_number = std::numeric_limits<uint64_t>::min(); | ||
| return s; | ||
| } | ||
|
|
||
| // Read the content of the temp file | ||
| std::ifstream temp_file(temp_file_path); | ||
| if (!temp_file.is_open()) { | ||
| Log(InfoLogLevel::ERROR_LEVEL, cfs_->info_log_, | ||
| "Failed to open temp file for reading smallest file number: %s, " | ||
| "object_key: %s", | ||
| temp_file_path.c_str(), object_key.c_str()); | ||
| *file_number = std::numeric_limits<uint64_t>::min(); | ||
| return Status::IOError("Failed to open temp file"); | ||
| } | ||
|
|
||
| std::string content((std::istreambuf_iterator<char>(temp_file)), | ||
| std::istreambuf_iterator<char>()); | ||
|
|
||
| temp_file.close(); | ||
| // Remove the temp file | ||
| if (std::remove(temp_file_path.c_str()) != 0) { | ||
| Log(InfoLogLevel::WARN_LEVEL, cfs_->info_log_, | ||
| "Warning: Failed to remove temp file %s", temp_file_path.c_str()); | ||
| } | ||
|
|
||
| try { | ||
| *file_number = std::stoull(content); | ||
| Log(InfoLogLevel::INFO_LEVEL, nullptr, | ||
| "Read smallest file number from S3: %llu, object_key: %s", | ||
| static_cast<unsigned long long>(*file_number), object_key.c_str()); | ||
| return Status::OK(); | ||
| } catch (const std::exception &e) { | ||
| Log(InfoLogLevel::ERROR_LEVEL, nullptr, | ||
| "Failed to parse smallest file number from S3 content: '%s', " | ||
| "returning UINT64_MIN", | ||
| content.c_str()); | ||
| *file_number = std::numeric_limits<uint64_t>::min(); | ||
| return Status::Corruption("Failed to parse smallest file number: %s", | ||
| e.what()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Header contract vs implementation for “not found.”
cloud/eloq_purger.h (lines 47–68) says ReadSmallestFileNumber returns UINT64_MAX if not found. Implementation now returns Status::NotFound (after the fix above) and uses 0 as the “blocked” sentinel at the caller. Please reconcile doc vs behavior (update header or adjust code) to avoid confusion.
🤖 Prompt for AI Agents
In cloud/eloq_purger.cc lines 60–125, the implementation currently uses
UINT64_MIN/0 as the "not found"/blocked sentinel and returns different Statuses
than the header contract; reconcile by making the implementation follow the
header: when the S3 object is missing or cannot be parsed, set *file_number =
std::numeric_limits<uint64_t>::max() (UINT64_MAX) and return Status::NotFound(),
update the error/log messages accordingly, and update any callers that treat 0
as the sentinel to instead check for UINT64_MAX; alternatively, if you prefer
the current behavior, update cloud/eloq_purger.h to document returning
Status::NotFound and using 0 as the sentinel—pick one consistent approach and
apply it across header, implementation, and callers.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cloud/eloq_purger.h (1)
46-48: Fix the class brief (reader vs writer).This is a reader, not a writer.
Apply:
- * @brief S3 file updater for writing smallest file number to S3 + * @brief S3 reader for reading the smallest file-number threshold from S3cloud/eloq_purger.cc (2)
23-23: Drop unused dependency on gflags in library file.This pulls in an unnecessary link-time dependency. The CLI uses gflags; the library shouldn’t.
-#include <gflags/gflags.h>
426-431: Fix misleading log when threshold entry is missing.Missing map entry ≠ “threshold is 0.” Clarify message.
- Log(InfoLogLevel::INFO_LEVEL, cfs_->info_log_, - "[pg] threshold is 0 for epoch %s, purge is blocked intentionally. " - "%s is skipped.", - candidate_epoch.c_str(), candidate_file_path.c_str()); + Log(InfoLogLevel::INFO_LEVEL, cfs_->info_log_, + "[pg] No threshold loaded for epoch %s; purge is blocked intentionally. %s is skipped.", + candidate_epoch.c_str(), candidate_file_path.c_str());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cloud/eloq_purger.cc(1 hunks)cloud/eloq_purger.h(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-17T09:48:20.191Z
Learnt from: githubzilla
PR: eloqdata/rocksdb-cloud#8
File: cloud/eloq_purger.cc:394-418
Timestamp: 2025-09-17T09:48:20.191Z
Learning: In the EloqPurger implementation in cloud/eloq_purger.cc, when the file number threshold is zero (UINT64_MIN), it intentionally blocks the purger as a safety mechanism. The comparison `file_number < threshold` when threshold is 0 will never be true for valid file numbers, effectively preventing any file deletion. This is by design, not an error condition.
Applied to files:
cloud/eloq_purger.hcloud/eloq_purger.cc
🧬 Code graph analysis (2)
cloud/eloq_purger.h (1)
cloud/eloq_purger.cc (26)
PrerequisitesMet(492-505)PrerequisitesMet(492-492)S3FileNumberReader(53-60)ReadSmallestFileNumber(62-127)ReadSmallestFileNumber(62-62)GetS3ObjectKey(129-137)GetS3ObjectKey(129-129)EloqPurger(139-144)RunSinglePurgeCycle(146-219)RunSinglePurgeCycle(146-146)ListAllFiles(221-235)ListAllFiles(221-221)ListCloudManifests(237-252)ListCloudManifests(237-238)LoadCloudManifests(254-294)LoadCloudManifests(254-256)CollectLiveFiles(296-347)CollectLiveFiles(296-298)LoadFileNumberThresholds(349-377)LoadFileNumberThresholds(349-351)SelectObsoleteSSTFilesWithThreshold(379-433)SelectObsoleteSSTFilesWithThreshold(379-382)SelectObsoleteManifestFiles(435-460)SelectObsoleteManifestFiles(435-438)DeleteObsoleteFiles(462-489)DeleteObsoleteFiles(462-463)
cloud/eloq_purger.cc (3)
cloud/eloq_purger.h (2)
S3FileNumberReader(48-69)EloqPurger(74-131)cloud/filename.h (2)
IsSstFile(146-156)IsManifestFile(170-183)cloud/eloq_purger_command.cc (1)
purger(325-326)
🔇 Additional comments (2)
cloud/eloq_purger.cc (2)
360-371: Don’t abort purge cycle on missing/unreadable thresholds; block that epoch and continue.Keeps other epochs purging and aligns with “threshold 0 = intentionally blocked.”
Apply:
- Status s = s3_updater->ReadSmallestFileNumber(&threshold); - if (!s.ok()) { - Log(InfoLogLevel::ERROR_LEVEL, cfs_->info_log_, - "[pg] Failed to read file number threshold for epoch %s: %s", - epoch.c_str(), s.ToString().c_str()); - return s; - } + Status s = s3_updater->ReadSmallestFileNumber(&threshold); + if (!s.ok()) { + if (s.IsNotFound() || s.IsCorruption()) { + Log(InfoLogLevel::WARN_LEVEL, cfs_->info_log_, + "[pg] Threshold missing/unparseable for epoch %s: %s; using UINT64_MIN (blocked)", + epoch.c_str(), s.ToString().c_str()); + threshold = std::numeric_limits<uint64_t>::min(); + } else { + Log(InfoLogLevel::ERROR_LEVEL, cfs_->info_log_, + "[pg] Failed to read threshold for epoch %s: %s; blocking this epoch and continuing", + epoch.c_str(), s.ToString().c_str()); + threshold = std::numeric_limits<uint64_t>::min(); + } + }
69-76: Fix Status/IOStatus mismatches, cleanup temp file on failures, and replace nullptr logger
- Replace all "return s;" (rocksdb::IOStatus) with appropriate Status conversions — occurrences: cloud/eloq_purger.cc:88 and 366.
- Unlink the temp file on every early return and stop mutating *file_number on error; return an error Status instead.
- Replace Log(..., nullptr) with Log(..., cfs_->info_log_) — occurrences: cloud/eloq_purger.cc:114 and 119.
- Remove or justify the unused #include <gflags/gflags.h> at cloud/eloq_purger.cc:23.
Apply the diff below (unchanged from original suggestion):
int fd = mkstemp(tmp_template); if (fd == -1) { Log(InfoLogLevel::ERROR_LEVEL, cfs_->info_log_, "Failed to create temp file for reading smallest file number from S3: " "%s, object_key: %s", strerror(errno), object_key.c_str()); - *file_number = std::numeric_limits<uint64_t>::min(); return Status::IOError("Failed to create temp file"); } @@ rocksdb::IOStatus s = cfs_->GetStorageProvider()->GetCloudObject( bucket_name_, object_key, temp_file_path); if (!s.ok()) { - Log(InfoLogLevel::ERROR_LEVEL, cfs_->info_log_, - "Failed to read smallest file number from S3: %s, object_key: %s, ", - s.ToString().c_str(), object_key.c_str()); - *file_number = std::numeric_limits<uint64_t>::min(); - return s; + Log(InfoLogLevel::WARN_LEVEL, cfs_->info_log_, + "Failed to read smallest file number from S3: %s, object_key: %s", + s.ToString().c_str(), object_key.c_str()); + std::remove(temp_file_path.c_str()); + if (s.IsNotFound()) { + return Status::NotFound("Threshold object not found"); + } + return Status::IOError(s.ToString()); } @@ std::ifstream temp_file(temp_file_path); if (!temp_file.is_open()) { Log(InfoLogLevel::ERROR_LEVEL, cfs_->info_log_, "Failed to open temp file for reading smallest file number: %s, " "object_key: %s", temp_file_path.c_str(), object_key.c_str()); - *file_number = std::numeric_limits<uint64_t>::min(); + std::remove(temp_file_path.c_str()); return Status::IOError("Failed to open temp file"); } @@ try { *file_number = std::stoull(content); - Log(InfoLogLevel::INFO_LEVEL, nullptr, + Log(InfoLogLevel::INFO_LEVEL, cfs_->info_log_, "Read smallest file number from S3: %llu, object_key: %s", static_cast<unsigned long long>(*file_number), object_key.c_str()); return Status::OK(); } catch (const std::exception &e) { - Log(InfoLogLevel::ERROR_LEVEL, nullptr, - "Failed to parse smallest file number from S3 content: '%s', " - "returning UINT64_MIN", - content.c_str()); - *file_number = std::numeric_limits<uint64_t>::min(); + Log(InfoLogLevel::WARN_LEVEL, cfs_->info_log_, + "Failed to parse smallest file number from S3 content: '%s'", + content.c_str()); return Status::Corruption("Failed to parse smallest file number: %s", e.what()); }
Summary by CodeRabbit
New Features
Improvements
Chores