feat: add eloq_store_root_meta_cache_size to eloq_store_config#388
Merged
Conversation
WalkthroughAdded a global configuration flag to control EloqStore root metadata cache size and integrated size parsing, validation against available memory, and adjustments to node memory accounting within EloqStoreConfig. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
thweetkomputer
changed the title
…ubmodule at 8617e36
![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
🤖 Fix all issues with AI agents
In `@store_handler/eloq_data_store_service/eloq_store_config.cpp`:
- Around line 365-396: The auto-sizing logic for root_meta_cache_size
(root_meta_cache_size_str, computed from node_memory_mb / 20) can produce "0MB"
on tiny nodes; change the auto-size branch to clamp the computed value to a
minimum of 1MB (i.e., compute auto_mb = std::max(node_memory_mb / 20, 1) and use
std::to_string(auto_mb) + "MB"), keep using parse_size(root_meta_cache_size_str)
and the existing FATAL check, update the logged message to report the clamped
value, and ensure eloqstore_configs_.root_meta_cache_size is set from the parsed
(clamped) value.
🧹 Nitpick comments (1)
store_handler/eloq_data_store_service/eloq_store_config.cpp (1)
77-79: Clarify accepted size units in the flag description.Small doc tweak reduces misconfiguration risk when users pass values.
🔧 Suggested doc tweak
-DEFINE_string(eloq_store_root_meta_cache_size, - "1GB", - "EloqStore RootMeta mappings cache size (global)."); +DEFINE_string(eloq_store_root_meta_cache_size, + "1GB", + "EloqStore RootMeta mappings cache size (global, e.g., 512MB/1GB).");
Comment on lines
+365
to
+396
| std::string root_meta_cache_size_str; | ||
| if (CheckCommandLineFlagIsDefault("eloq_store_root_meta_cache_size")) | ||
| { | ||
| if (config_reader.HasValue("store", "eloq_store_root_meta_cache_size")) | ||
| { | ||
| root_meta_cache_size_str = | ||
| config_reader.GetString("store", | ||
| "eloq_store_root_meta_cache_size", | ||
| FLAGS_eloq_store_root_meta_cache_size); | ||
| } | ||
| else | ||
| { | ||
| root_meta_cache_size_str = | ||
| std::to_string(node_memory_mb / 20) + "MB"; | ||
| LOG(INFO) << "config is automatically set: " | ||
| << "eloq_store_root_meta_cache_size=" | ||
| << root_meta_cache_size_str | ||
| << ", available memory=" << node_memory_mb << "MB"; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| root_meta_cache_size_str = FLAGS_eloq_store_root_meta_cache_size; | ||
| } | ||
| uint64_t root_meta_cache_size = parse_size(root_meta_cache_size_str); | ||
| if (root_meta_cache_size / (1024 * 1024) > node_memory_mb) | ||
| { | ||
| LOG(FATAL) << "root meta cache size (" << root_meta_cache_size | ||
| << ") exceeds node memory mb"; | ||
| } | ||
| node_memory_mb -= root_meta_cache_size / (1024 * 1024); | ||
| eloqstore_configs_.root_meta_cache_size = root_meta_cache_size; |
There was a problem hiding this comment.
Guard the auto-sized cache against 0MB when memory is tiny.
node_memory_mb / 20 can yield 0MB for small test/dev nodes, silently disabling the cache. Consider clamping to a minimum to avoid surprising behavior.
🛠️ Proposed fix (min 1MB)
- root_meta_cache_size_str =
- std::to_string(node_memory_mb / 20) + "MB";
+ const uint32_t auto_root_meta_mb =
+ std::max<uint32_t>(1, node_memory_mb / 20);
+ root_meta_cache_size_str =
+ std::to_string(auto_root_meta_mb) + "MB";🤖 Prompt for AI Agents
In `@store_handler/eloq_data_store_service/eloq_store_config.cpp` around lines 365
- 396, The auto-sizing logic for root_meta_cache_size (root_meta_cache_size_str,
computed from node_memory_mb / 20) can produce "0MB" on tiny nodes; change the
auto-size branch to clamp the computed value to a minimum of 1MB (i.e., compute
auto_mb = std::max(node_memory_mb / 20, 1) and use std::to_string(auto_mb) +
"MB"), keep using parse_size(root_meta_cache_size_str) and the existing FATAL
check, update the logged message to report the clamped value, and ensure
eloqstore_configs_.root_meta_cache_size is set from the parsed (clamped) value.
thweetkomputer
approved these changes
Jan 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Here are some reminders before you submit the pull request
fixes eloqdb/tx_service#issue_id./mtr --suite=mono_main,mono_multi,mono_basicSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.