Skip to content

Add disable write stall parameter#153

Open
githubzilla wants to merge 1 commit intomainfrom
disable_write_stall
Open

Add disable write stall parameter#153
githubzilla wants to merge 1 commit intomainfrom
disable_write_stall

Conversation

@githubzilla
Copy link
Collaborator

@githubzilla githubzilla commented Nov 21, 2025

Summary by CodeRabbit

  • New Features
    • Added rocksdb_disable_write_stall configuration option, enabling users to control RocksDB write stall behavior for improved database performance and throughput optimization during peak workloads. This new setting is fully configurable via command-line arguments or INI configuration files, and defaults to disabled for maximum backward compatibility with existing system configurations and deployments.

✏️ Tip: You can customize this high-level summary in your review settings.

@githubzilla githubzilla requested a review from zhangh43 November 21, 2025 10:04
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 21, 2025

Walkthrough

A new RocksDB configuration option disable_write_stall is introduced through the configuration system. The flag can be set via command-line or INI config and is propagated from RocksDBConfig through RocksDBDataStoreCommon to be applied as a RocksDB option during database initialization.

Changes

Cohort / File(s) Summary
Configuration Flag Definition
eloq_data_store_service/rocksdb_config.cpp
Adds global boolean flag rocksdb_disable_write_stall (default: false) and initializes new disable_write_stall_ member in constructor, reading from CLI flag with INI config fallback.
Configuration Structure
eloq_data_store_service/rocksdb_config.h
Adds public boolean member variable disable_write_stall_ to RocksDBConfig struct.
Data Store Configuration
eloq_data_store_service/rocksdb_data_store_common.h
Adds protected const boolean member disable_write_stall_ and initializes it from config.disable_write_stall_ in constructor initializer list.
RocksDB Option Application
eloq_data_store_service/rocksdb_cloud_data_store.cpp
Adds conditional logic in OpenCloudDB() to set options.disable_write_stall when disable_write_stall_ is true, positioned before purger event listener attachment.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Homogeneous changes following existing configuration patterns across files
  • New option flows consistently through config → data store common → cloud data store
  • Single conditional application in OpenCloudDB with no complex logic
  • Areas of focus: Verify the option is applied at the correct point in the initialization sequence and that initialization order doesn't affect subsequent RocksDB option setup

Possibly related PRs

Suggested reviewers

  • liunyl

Poem

🐰 A write stall's been tamed with a flag so divine,
Through config it flows in a steady line,
From CLI to INI, then into the store,
RocksDB options bloom—what could be more?

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 accurately summarizes the main change: adding a new disable_write_stall parameter across the configuration and RocksDB data store components.
✨ 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 disable_write_stall

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


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.

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_cloud_data_store.cpp (1)

599-602: Simplify the conditional logic for boolean option.

The conditional block is unnecessary for a boolean configuration. Other boolean options in this file (e.g., unordered_write, paranoid_checks, atomic_flush) are set unconditionally. The current pattern only makes sense for numeric values where 0 indicates "use RocksDB default."

Apply this diff:

-    if (disable_write_stall_)
-    {
-        options.disable_write_stall = disable_write_stall_;
-    }
+    options.disable_write_stall = disable_write_stall_;
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d584a13 and d6effec.

📒 Files selected for processing (4)
  • eloq_data_store_service/rocksdb_cloud_data_store.cpp (1 hunks)
  • eloq_data_store_service/rocksdb_config.cpp (2 hunks)
  • eloq_data_store_service/rocksdb_config.h (1 hunks)
  • eloq_data_store_service/rocksdb_data_store_common.h (2 hunks)
🧰 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 (5)
eloq_data_store_service/rocksdb_config.h (1)

69-69: LGTM - clean config member addition.

The new disable_write_stall_ member is properly declared and follows the existing struct conventions.

eloq_data_store_service/rocksdb_config.cpp (2)

101-104: LGTM - proper flag definition.

The flag is correctly defined with a conservative default value (false) and clear documentation.


496-501: LGTM - consistent initialization pattern.

The initialization follows the established pattern: CLI flag takes precedence, falls back to INI config, and uses the flag default as the ultimate fallback.

eloq_data_store_service/rocksdb_data_store_common.h (1)

144-144: LGTM - proper member initialization and declaration.

The disable_write_stall_ member is correctly initialized from the config and declared as a protected const member, following the established pattern for other configuration fields.

Also applies to: 311-311

eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)

599-602: Verification confirms RocksDB API compatibility; alternative flow control exists in codebase.

The disable_write_stall option is confirmed to exist in RocksDB. The code is properly configured with:

  • Default disabled (safe-by-default via DEFINE_bool(..., false, ...))
  • Defensive assignment pattern (only applied if explicitly enabled)
  • Alternative flow control mechanism in place: write_rate_limiter is configured and applied via rocksdb::NewGenericRateLimiter()

Before enabling this setting, verify that:

  1. Your workload requires disabling write stalls (not a performance concern with rate limiting alone)
  2. You have monitoring in place for the risks: unbounded write accumulation, disk exhaustion, and read/compaction performance degradation
  3. The rate limiter configuration is tuned appropriately for your throughput requirements

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.

1 participant