Skip to content

Add run_purger parameter#98

Merged
githubzilla merged 1 commit intomainfrom
eloq_purger_test
Oct 9, 2025
Merged

Add run_purger parameter#98
githubzilla merged 1 commit intomainfrom
eloq_purger_test

Conversation

@githubzilla
Copy link
Collaborator

@githubzilla githubzilla commented Sep 29, 2025

Summary by CodeRabbit

  • New Features

    • Added a configurable option to enable or disable the background purger for the cloud-backed data store.
    • Can be controlled via command-line flag or config, defaulting to enabled; startup now honors this setting.
  • Chores

    • Propagated the purger setting through configuration and initialization so runtime behavior respects the chosen value.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 29, 2025

Walkthrough

StartDB's purger startup is now configurable via a new RocksDBCloudConfig::run_purger_ flag (default true) surfaced as a command-line/config flag rocksdb_cloud_run_purger. StartDB reads cloud_config_.run_purger_ and conditionally initializes the purger instead of always running it.

Changes

Cohort / File(s) Summary of Changes
Config plumbing: flag + struct field
eloq_data_store_service/rocksdb_config.h, eloq_data_store_service/rocksdb_config.cpp
Added bool run_purger_ { true }; to RocksDBCloudConfig. Introduced DEFINE_bool(rocksdb_cloud_run_purger, true, ...), read flag/INI value, and assign to run_purger_.
Startup behavior: conditional purger
eloq_data_store_service/rocksdb_cloud_data_store.cpp
StartDB now reads cloud_config_.run_purger_ to decide whether to initialize/run the purger instead of hardcoding it to run. Control flow and error handling around purge setup adjusted accordingly.

Sequence Diagram(s)

sequenceDiagram
    participant CLI as CLI / INI
    participant Cfg as RocksDBCloudConfig
    participant DS as RocksDBCloudDataStore::StartDB
    participant Purger as Purger

    CLI->>Cfg: parse rocksdb_cloud_run_purger (default true)
    Note right of Cfg: set run_purger_
    Cfg->>DS: StartDB(cloud_config)
    alt run_purger_ == true
        DS->>Purger: initialize & start purger
        Note over DS,Purger: purger active during startup/runtime
    else run_purger_ == false
        DS--xPurger: skip purger setup
        Note over DS: proceed without purger
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • liunyl
  • thweetkomputer

Poem

A rabbit hops through config fields,
A tiny flag, the code it yields.
Purger waits or springs to play,
Clouds keep tidy, day by day.
Hop—deploy—then nibble bugs away. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title “Add run_purger parameter” directly reflects the primary change in this pull request, which is introducing a configurable run_purger flag to the RocksDB cloud configuration, and is concise and specific enough for teammates to understand the main intent at a glance.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch eloq_purger_test

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@githubzilla githubzilla merged commit 61c7c80 into main Oct 9, 2025
1 check was pending
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
eloq_data_store_service/rocksdb_config.cpp (1)

275-277: LGTM! Flag definition is correct.

The flag definition follows the standard pattern with a sensible default of true (preserving previous behavior). The description is adequate, though you could optionally enhance it to something like "Enable/disable RocksDB cloud storage purger for obsolete file cleanup" for better clarity.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 56016a4 and c2e5bce.

📒 Files selected for processing (3)
  • eloq_data_store_service/rocksdb_cloud_data_store.cpp (2 hunks)
  • eloq_data_store_service/rocksdb_config.cpp (3 hunks)
  • eloq_data_store_service/rocksdb_config.h (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • eloq_data_store_service/rocksdb_config.h
🧰 Additional context used
🧬 Code graph analysis (1)
eloq_data_store_service/rocksdb_config.cpp (1)
rocksdb_config.cpp (2)
  • CheckCommandLineFlagIsDefault (298-310)
  • CheckCommandLineFlagIsDefault (298-298)
🔇 Additional comments (3)
eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)

325-325: LGTM! Purger is now configurable.

The change correctly sources the run_purger flag from cloud_config_.run_purger_ instead of hardcoding it to true. This makes the purger startup behavior configurable as intended, and the value is properly logged on line 338 for observability.

eloq_data_store_service/rocksdb_config.cpp (2)

561-566: LGTM! Flag reading follows the established pattern.

The implementation correctly reads the flag value with the standard precedence: command-line flag → INI config → default value. This matches the pattern used for other configuration flags throughout the file.


581-581: LGTM! Assignment completes the configuration flow.

The assignment correctly stores the resolved flag value to the run_purger_ member, which is then consumed by RocksDBCloudDataStore::StartDB() to control purger initialization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants