fix(storage): share block cache across instances when share-block-cache is enabled (#143)

[AlexStocks]

Summary

Fixes #143 — the block-cache sharing was effectively dead code due to an inverted condition.

With the default share_block_cache = true, the old code did not create any shared cache:

if !storage_options.share_block_cache && storage_options.block_cache_size > 0 { ... }

so every RocksDB instance silently fell back to its own isolated (RocksDB-default) cache. This defeated the intent of sharing and wasted memory.

Changes

• conf (config.rs): add the share-block-cache config key (default true) plus unit tests covering default / yes / no / invalid values. The new field is also serialized into the sample config automatically via to_redis_style.
• storage (options.rs): build the shared Arc<Cache> once in StorageOptions::from_config and store it on StorageOptions, so every Redis instance reuses the same cache through the shared Arc<StorageOptions>.
• storage (redis.rs): prefer the shared cache; only fall back to a per-instance cache when sharing is disabled and a block_cache_size is configured.

Behavior

• share-block-cache yes (default): one cache of memory bytes is built once and shared across all instances/CFs.
• share-block-cache no: each instance builds its own independent cache of block_cache_size bytes.
• StorageOptions::default() keeps block_cache: None, so existing unit tests are unaffected.

Test plan

• cargo test -p conf — 30 passed (incl. 3 new share-block-cache parse tests)
• cargo check -p storage --tests --features test-fault-injection — clean
• cargo clippy -p conf -p storage --tests --features test-fault-injection — no new warnings from changed files

Fixes #143

Summary by CodeRabbit

• New Features

  • Added a configuration option to share the storage block cache across instances.
  • Supports yes and no values, with validation for invalid settings.
  • Shared caching is enabled by default when cache memory is configured.

• Performance

  • Improved cache reuse by allowing storage instances to share a common block cache.
  • Preserved independent caching behavior when shared caching is disabled.

[coderabbitai]

Warning

Review limit reached

@AlexStocks, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 10 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 64a83b3e-862a-4194-a6f7-c90ad6bad60d

📥 Commits

Reviewing files that changed from the base of the PR and between 2d5680b and 9d1f833.

📒 Files selected for processing (5)

• src/raft/src/state_machine.rs
• src/raft/tests/snapshot_roundtrip_test.rs
• src/storage/src/options.rs
• src/storage/src/redis.rs
• src/storage/src/storage.rs

📝 Walkthrough

Walkthrough

Changes

Shared block cache

Layer / File(s)	Summary
Configuration contract and validation src/conf/src/config.rs, src/conf/src/lib.rs	Adds Config::share_block_cache, enables it by default, parses share-block-cache, reports invalid values, and updates test fixtures.
Shared cache creation and RocksDB wiring src/storage/src/options.rs, src/storage/src/redis.rs	Creates an optional shared LRU cache and applies it to RocksDB column-family options, with per-instance fallback.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Config
  participant StorageOptions
  participant ColumnFamilyOptions
  participant RocksDB

  Config->>StorageOptions: provide share_block_cache and cache size
  StorageOptions->>StorageOptions: create shared LRU cache when enabled
  StorageOptions->>ColumnFamilyOptions: pass optional block cache
  ColumnFamilyOptions->>RocksDB: configure shared or per-instance cache

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the storage fix and the configuration condition that enables shared block-cache behavior.
Description check	✅ Passed	The description explains the fix, references issue #143, details the implementation, and documents the completed test plan.
Linked Issues check	✅ Passed	The changes satisfy issue #143 by adding configuration parsing, creating one shared cache, and using per-instance fallback when sharing is disabled.
Out of Scope Changes check	✅ Passed	All changes support block-cache configuration, construction, application, or testing, with no unrelated code changes identified.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/issue-143-shared-block-cache

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/storage/src/options.rs (1)

133-140: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Copy share_block_cache from Config.

Line 140 retains StorageOptions::default().share_block_cache == true. If the configuration sets share-block-cache no, block_cache is None, but src/storage/src/redis.rs lines 487-490 skip the per-instance fallback because the flag is still true.

Set share_block_cache: config.share_block_cache in this initializer. Add a regression test for share-block-cache no.

Based on PR objectives, disabled sharing must use a per-instance cache.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/storage/src/options.rs` around lines 133 - 140, Propagate
config.share_block_cache into the StorageOptions initializer so the configured
sharing mode overrides the default. Add a regression test covering
share-block-cache no and verify the resulting Redis/storage path uses a
per-instance cache when sharing is disabled.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/storage/src/redis.rs`:
- Around line 477-491: Create the non-shared fallback block cache once in
Redis::open when block_cache_size is positive, then pass that cache to every
create_cf_options call for the Redis instance. Update create_cf_options to
accept and reuse the per-instance cache, removing its per-column-family cache
allocation while preserving shared-cache behavior through
StorageOptions::block_cache.

---

Outside diff comments:
In `@src/storage/src/options.rs`:
- Around line 133-140: Propagate config.share_block_cache into the
StorageOptions initializer so the configured sharing mode overrides the default.
Add a regression test covering share-block-cache no and verify the resulting
Redis/storage path uses a per-instance cache when sharing is disabled.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 266bcaa0-b32b-4953-b885-e08018f9be90

📥 Commits

Reviewing files that changed from the base of the PR and between 3164d4a and 2d5680b.

📒 Files selected for processing (4)

• src/conf/src/config.rs
• src/conf/src/lib.rs
• src/storage/src/options.rs
• src/storage/src/redis.rs

[Copilot]

🟡 Not ready to approve

StorageOptions::from_config does not propagate config.share_block_cache into StorageOptions, breaking the documented share-block-cache no behavior (no shared cache and no per-instance caches).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Fixes the storage block-cache sharing logic and adds configuration support so RocksDB instances can correctly share a single block cache when share-block-cache is enabled (default), aligning behavior with the intended memory-efficiency and cache-hit improvements.

Changes:

• Added share-block-cache config key (default yes) with parsing + unit tests.
• Built a shared Arc<Cache> once in StorageOptions::from_config and plumbed it into per-instance RocksDB table options.
• Updated RocksDB table options setup to prefer the shared cache and only create per-instance caches when sharing is disabled.

File summaries

File	Description
src/storage/src/redis.rs	Uses StorageOptions.block_cache when present; otherwise optionally builds a per-instance cache when sharing is disabled.
src/storage/src/options.rs	Adds block_cache: Option<Arc<Cache>> and builds the shared cache in from_config.
src/conf/src/lib.rs	Updates a test config struct literal to include share_block_cache.
src/conf/src/config.rs	Adds share_block_cache field, parsing for share-block-cache, and unit tests for default/yes/no/invalid values.

Review details

Suppressed comments (1)

src/storage/src/options.rs:140

• StorageOptions::from_config computes block_cache based on config.share_block_cache, but it never copies config.share_block_cache into the returned StorageOptions (it relies on ..Self::default(), which sets share_block_cache: true). As a result, share-block-cache no will produce StorageOptions { share_block_cache: true, block_cache: None }, and Redis will neither use a shared cache nor create per-instance caches (because the per-instance branch is gated by !storage_options.share_block_cache).

            block_cache,
            small_compaction_threshold: config.small_compaction_threshold,
            small_compaction_duration_threshold: config.small_compaction_duration_threshold,
            db_instance_num: config.db_instance_num,
            ..Self::default()

• Files reviewed: 4/4 changed files
• Comments generated: 0
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.