Disable auto compaction at the 1st place to make sure purger blocking…#92
Conversation
WalkthroughAdds an epoch-inclusive log in the purger and introduces temporary disabling of RocksDB auto compactions during database open, then re-enables them via SetOptions after coordinating with the purger. No public APIs changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant DS as RocksDBCloudDataStore
participant RDB as RocksDB
participant Purger
App->>DS: Open()
rect rgba(200,220,255,0.25)
note over DS: Pre-open coordination
DS->>DS: options.disable_auto_compactions = true
end
DS->>RDB: Open(options)
RDB-->>DS: Opened
rect rgba(200,255,200,0.25)
note over DS,Purger: Coordinate purger/background work
DS->>Purger: Block/await purger-safe state
Purger-->>DS: Purger blocked
DS->>RDB: SetOptions(disable_auto_compactions=false)
end
DS-->>App: Open() complete
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
eloq_data_store_service/purger_sliding_window.cpp(1 hunks)eloq_data_store_service/rocksdb_cloud_data_store.cpp(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-17T08:10:28.801Z
Learnt from: githubzilla
PR: eloqdata/store_handler#81
File: eloq_data_store_service/purger_sliding_window.cpp:240-276
Timestamp: 2025-09-17T08:10:28.801Z
Learning: In the EloqDS purger sliding window implementation (purger_sliding_window.cpp), entries in the sliding window must be explicitly removed via RemoveFileNumber() and do not expire automatically based on time. The entry_duration_ parameter only applies to deleted entries to prevent frequent S3 updates, not for automatic expiration of active entries.
Applied to files:
eloq_data_store_service/purger_sliding_window.cpp
🔇 Additional comments (1)
eloq_data_store_service/purger_sliding_window.cpp (1)
131-133: LGTM: epoch added to purger-block log improves traceabilityThe added epoch in the log line is helpful for debugging and correlating purger coordination.
| // Disable auto compactions before blocking purger | ||
| options.disable_auto_compactions = true; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Verify scope: disable_auto_compactions here affects only default CF
disable_auto_compactions is a ColumnFamily option. Setting it on Options before open typically covers only the default CF; existing CFs opened from manifest may keep their own settings. If multiple CFs exist, auto compactions may still run for them until PauseBackgroundWork executes.
Consider:
- If you have multiple CFs: open with explicit ColumnFamilyDescriptors, setting disable_auto_compactions = true per CF; or immediately after open, call SetOptions on every CF handle before any background work resumes.
Run this to locate CF usage and confirm single-CF assumption:
🏁 Script executed:
#!/bin/bash
# Scan for column family usage patterns
rg -n -C2 -g '!**/build/**' -P --type=cpp '\b(CreateColumnFamily|ListColumnFamilies|DefaultColumnFamily\(|ColumnFamilyHandle\b|GetColumnFamilyHandle\b)'Length of output: 148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "Top-level listing:"
ls -la
TARGET="eloq_data_store_service/rocksdb_cloud_data_store.cpp"
echo
echo "Check target: $TARGET"
if [ -f "$TARGET" ]; then
echo "FOUND $TARGET"
echo
echo "Print 1-220:"
sed -n '1,220p' "$TARGET" || true
echo "----"
echo "Print 560-720 (area around line ~610):"
sed -n '560,720p' "$TARGET" || true
else
echo "MISSING $TARGET"
fi
echo
echo "rg: searching for RocksDB ColumnFamily usage and related symbols (forced, include ignored files)"
rg -uu -n -S 'ColumnFamilyHandle|CreateColumnFamily|ListColumnFamilies|GetColumnFamilyHandle|DefaultColumnFamily|ColumnFamilyDescriptor|ColumnFamilyOptions' || true
echo
echo "rg: searching for disable_auto_compactions occurrences"
rg -uu -n -S 'disable_auto_compactions' || trueLength of output: 21574
Apply disable_auto_compactions per-column-family (Options covers only default CF)
disable_auto_compactions is a ColumnFamily option — setting options.disable_auto_compactions before DB::Open only affects the default CF. This repo manages multiple CFs (rocksdb_handler.cpp), so other CFs can still perform compactions between Open and PauseBackgroundWork.
Action: either open with ColumnFamilyDescriptor(s) that set disable_auto_compactions=true for every CF, or immediately after Open and before ContinueBackgroundWork call SetOptions on every ColumnFamilyHandle to disable compactions (and re-enable later).
Locations: eloq_data_store_service/rocksdb_cloud_data_store.cpp (disable true ~line 611; SetOptions(false) ~line 691). Inspect rocksdb_handler.cpp for CF creation/listing.
🤖 Prompt for AI Agents
In eloq_data_store_service/rocksdb_cloud_data_store.cpp around lines 610-612 and
691, disable_auto_compactions is being set only on the global Options (affects
default CF only) so non-default column families can still compact; update the
code to apply disable_auto_compactions to every CF: either construct
ColumnFamilyDescriptor entries for each column family (see rocksdb_handler.cpp
for CF names/creation) and set disable_auto_compactions=true on each descriptor
before DB::Open, or immediately after DB::Open and before ContinueBackgroundWork
call SetOptions on every returned ColumnFamilyHandle to disable compactions (and
ensure you later call SetOptions to re-enable when unpausing). Ensure this
change targets all CFs and preserves the existing re-enable logic around line
~691.
| // Resume background work | ||
| db_->ContinueBackgroundWork(); | ||
|
|
||
| // Enable auto compactions after blocking purger | ||
| status = db_->SetOptions({{"disable_auto_compactions", "false"}}); | ||
|
|
There was a problem hiding this comment.
Critical: compactions may remain disabled — wrong SetOptions overload and ordering
- SetOptions without a ColumnFamilyHandle targets DBOptions; disable_auto_compactions is a CF option, so this likely no-ops/fails.
- Also enable compactions before resuming background work to avoid running with compactions disabled if SetOptions fails.
Apply this fix:
- // Resume background work
- db_->ContinueBackgroundWork();
-
- // Enable auto compactions after blocking purger
- status = db_->SetOptions({{"disable_auto_compactions", "false"}});
+ // Enable auto compactions after blocking purger (default column family)
+ status = db_->SetOptions(
+ db_->DefaultColumnFamily(),
+ {{"disable_auto_compactions", "false"}});
+ if (!status.ok())
+ {
+ LOG(ERROR) << "Fail to enable auto compactions, error: "
+ << status.ToString();
+ // Avoid leaving background work paused; resume and fail fast.
+ db_->ContinueBackgroundWork();
+ return false;
+ }
+ // Resume background work
+ db_->ContinueBackgroundWork();📝 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.
| // Resume background work | |
| db_->ContinueBackgroundWork(); | |
| // Enable auto compactions after blocking purger | |
| status = db_->SetOptions({{"disable_auto_compactions", "false"}}); | |
| // Enable auto compactions after blocking purger (default column family) | |
| status = db_->SetOptions( | |
| db_->DefaultColumnFamily(), | |
| {{"disable_auto_compactions", "false"}}); | |
| if (!status.ok()) | |
| { | |
| LOG(ERROR) << "Fail to enable auto compactions, error: " | |
| << status.ToString(); | |
| // Avoid leaving background work paused; resume and fail fast. | |
| db_->ContinueBackgroundWork(); | |
| return false; | |
| } | |
| // Resume background work | |
| db_->ContinueBackgroundWork(); |
🤖 Prompt for AI Agents
In eloq_data_store_service/rocksdb_cloud_data_store.cpp around lines 687-692,
the code calls db_->SetOptions(...) with a DB-only overload after resuming
background work, which doesn't set the column-family-level
"disable_auto_compactions" and may leave compactions disabled; change to call
the ColumnFamily-aware SetOptions (db_->SetOptions(column_family_handle,
{{"disable_auto_compactions","false"}})) before calling ContinueBackgroundWork,
check the returned status and handle/log any error (and avoid resuming
background work if the SetOptions fails).
… succeed
Summary by CodeRabbit
Bug Fixes
Chores