fix:block_cache_size#3169
Conversation
WalkthroughAdds tri-state block cache handling in src/pika_server.cc: disable cache when block_cache_size == 0, use shared global cache when share_block_cache is set (asserts shared cache initialized), or create a per-instance LRU cache otherwise; preserves rate limiter and other storage option setup. Changes
Sequence Diagram(s)sequenceDiagram
participant Server
participant StorageOptions
participant SharedCache as Shared Block Cache
participant LRU as Per-Instance LRU Cache
Server->>StorageOptions: InitializeStorageOptions()
alt block_cache_size == 0
StorageOptions->>StorageOptions: no_block_cache = true\nreset per-instance block_cache
else share_block_cache == true
StorageOptions->>SharedCache: assert initialized
StorageOptions->>StorageOptions: no_block_cache = false\nuse shared_block_cache_
else
StorageOptions->>LRU: create(size=block_cache_size)
StorageOptions->>StorageOptions: no_block_cache = false\nset per-instance block_cache
end
Server->>StorageOptions: apply rate_limiter and other options
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pika_server.cc(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_on_macos
- GitHub Check: build_on_ubuntu
- GitHub Check: build_on_centos
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
src/pika_server.cc (1)
1482-1496: LGTM! Block cache configuration logic is correctly implemented.The implementation properly handles all three cache modes with clear branching logic. The assertion for shared cache initialization is a good defensive programming practice to catch configuration errors early.
| if (storage_options_.block_cache_size == 0) { | ||
| storage_options_.table_options.no_block_cache = true; | ||
| storage_options_.table_options.block_cache.reset(); | ||
| } else if (storage_options_.share_block_cache) { | ||
| // 共享模式：直接复用已创建好的全局 cache（PikaServer 成员） | ||
| storage_options_.table_options.no_block_cache = false; | ||
| assert(share_block_cache_ && "shared_block_cache_ must be initialized before InitStorageOptions()"); | ||
| storage_options_.table_options.block_cache = shared_block_cache_; | ||
| } else { | ||
| // 非共享模式：为当前 CF/实例单独创建 | ||
| storage_options_.table_options.no_block_cache = false; | ||
| storage_options_.table_options.block_cache = | ||
| rocksdb::NewLRUCache(storage_options_.block_cache_size, | ||
| static_cast<int>(g_pika_conf->num_shard_bits())); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify shared_block_cache_ initialization order.
The code assumes shared_block_cache_ is initialized before InitStorageOptions() is called. Please verify that the shared cache is properly initialized in the constructor or startup sequence before this method is invoked.
🏁 Script executed:
#!/bin/bash
# Search for shared_block_cache_ initialization in the codebase
rg -n "shared_block_cache_.*=" --type cpp -A 3 -B 3
# Search for InitStorageOptions calls to understand initialization order
rg -n "InitStorageOptions" --type cpp -A 3 -B 3
# Look for shared_block_cache_ declaration
ast-grep --pattern 'shared_ptr<$_> shared_block_cache_'Length of output: 1539
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for share_block_cache_ / shared_block_cache_ occurrences..."
rg -n "share_block_cache_|shared_block_cache_" -S || true
echo
echo "Show declaration area in include/pika_server.h (lines 1-260):"
sed -n '1,260p' include/pika_server.h || true
echo
echo "Show top of src/pika_server.cc (lines 1-140) to see constructor/init order:"
sed -n '1,140p' src/pika_server.cc || true
echo
echo "Search for places that create or assign a block cache (NewLRUCache or MakeLRUCache or Assign to *_block_cache):"
rg -n "NewLRUCache|MakeLRUCache|_block_cache" -S || true
echo
echo "Search for functions/methods that might initialize a shared cache (InitShared|InitBlockCache|CreateSharedCache):"
rg -n "Init.*Block|Init.*Cache|Create.*Cache|shared.*cache" -S || trueLength of output: 126768
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search for 'shared_block_cache' variants across the repo:"
rg -n "shared_block_cache|share_block_cache_|shared_block_cache_" -S || true
echo
echo "Show remainder of include/pika_server.h to find member declarations (lines 260-999):"
sed -n '260,999p' include/pika_server.h || true
echo
echo "Search for any member declarations referencing rocksdb::Cache or Cache pointer in headers:"
rg -n "rocksdb::Cache|std::shared_ptr<.*Cache>" -S || true
echo
echo "Show PikaServer private members area by locating closing of class (search for 'private:' within header):"
rg -n "class PikaServer|private:" include/pika_server.h -n -n -S || true
sed -n '420,999p' include/pika_server.h || trueLength of output: 18930
Fix shared_block_cache_ initialization and correct the assert typo.
InitStorageOptions() is called from the constructor (src/pika_server.cc:57). Inside InitStorageOptions (src/pika_server.cc:1426-1490) you assign storage_options_.table_options.block_cache = shared_block_cache_ but there is no observable declaration/initialization of shared_block_cache_ before that call, and the assert checks share_block_cache_ (typo). Initialize shared_block_cache_ before InitStorageOptions() (or move the InitStorageOptions() call) and change the assert to assert(shared_block_cache_ && ...) or otherwise guard the assignment.
🤖 Prompt for AI Agents
In src/pika_server.cc around lines 1482-1496, InitStorageOptions() assigns
storage_options_.table_options.block_cache = shared_block_cache_ but
shared_block_cache_ is not guaranteed initialized and the assert erroneously
checks share_block_cache_; fix by correcting the assert to
assert(shared_block_cache_ && "shared_block_cache_ must be initialized before
InitStorageOptions()"), and ensure shared_block_cache_ is created/initialized
before InitStorageOptions() is called (either initialize it in the constructor
prior to calling InitStorageOptions() or move the InitStorageOptions() call
after shared_block_cache_ creation); alternatively add a runtime guard so you
only assign shared_block_cache_ when it is non-null to avoid dereferencing an
uninitialized pointer.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pika_server.cc(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_on_ubuntu
- GitHub Check: build_on_macos
- GitHub Check: build_on_centos
- GitHub Check: Analyze (go)
| } else if (storage_options_.share_block_cache) { | ||
| if (!g_pika_conf->share_block_cache()) { | ||
| assert(false && "shared_block_cache_ must be initialized before InitStorageOptions()"); | ||
| } | ||
|
|
||
| if (storage_options_.block_cache_size == 0) { | ||
| storage_options_.table_options.no_block_cache = true; | ||
| } else if (storage_options_.share_block_cache) { | ||
| storage_options_.table_options.block_cache = | ||
| rocksdb::NewLRUCache(storage_options_.block_cache_size, static_cast<int>(g_pika_conf->num_shard_bits())); | ||
| } | ||
| storage_options_.table_options.no_block_cache = false; | ||
|
|
||
| storage_options_.table_options.block_cache = 0; | ||
| } else { |
There was a problem hiding this comment.
Shared cache branch never attaches the shared cache
When share_block_cache is enabled we still leave block_cache as nullptr. RocksDB will silently fall back to its default per-instance cache instead of reusing the intended global cache, so the “shared” mode never actually shares anything. The guard also re-checks the config flag instead of validating that shared_block_cache_ has been initialized.
Wire the shared cache through (and assert it exists) before we flip no_block_cache off:
- } else if (storage_options_.share_block_cache) {
- if (!g_pika_conf->share_block_cache()) {
- assert(false && "shared_block_cache_ must be initialized before InitStorageOptions()");
- }
-
- storage_options_.table_options.no_block_cache = false;
-
- storage_options_.table_options.block_cache = 0;
+ } else if (storage_options_.share_block_cache) {
+ storage_options_.table_options.no_block_cache = false;
+ assert(shared_block_cache_ && "shared_block_cache_ must be initialized before InitStorageOptions()");
+ storage_options_.table_options.block_cache = shared_block_cache_;📝 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.
| } else if (storage_options_.share_block_cache) { | |
| if (!g_pika_conf->share_block_cache()) { | |
| assert(false && "shared_block_cache_ must be initialized before InitStorageOptions()"); | |
| } | |
| if (storage_options_.block_cache_size == 0) { | |
| storage_options_.table_options.no_block_cache = true; | |
| } else if (storage_options_.share_block_cache) { | |
| storage_options_.table_options.block_cache = | |
| rocksdb::NewLRUCache(storage_options_.block_cache_size, static_cast<int>(g_pika_conf->num_shard_bits())); | |
| } | |
| storage_options_.table_options.no_block_cache = false; | |
| storage_options_.table_options.block_cache = 0; | |
| } else { | |
| } else if (storage_options_.share_block_cache) { | |
| storage_options_.table_options.no_block_cache = false; | |
| assert(shared_block_cache_ && "shared_block_cache_ must be initialized before InitStorageOptions()"); | |
| storage_options_.table_options.block_cache = shared_block_cache_; | |
| } else { |
🤖 Prompt for AI Agents
In src/pika_server.cc around lines 1485-1493, the shared-cache branch flips
table_options.no_block_cache to false but leaves table_options.block_cache as
nullptr and re-checks g_pika_conf instead of validating the actual
shared_block_cache_ object; update this branch to assert that
shared_block_cache_ is initialized (not the config flag) and assign the shared
cache to storage_options_.table_options.block_cache before clearing
no_block_cache so RocksDB uses the intended global cache.
2 participants
Summary by CodeRabbit
New Features
Improvements