chore: update eloqstore parameters and fix index operation destructor#391
Merged
Conversation
WalkthroughUpdated the Changes
Sequence Diagram(s)(No sequence diagrams generated — changes do not introduce a new multi-component sequential flow that requires visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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
force-pushed
the
update-submodule-zc
branch
2 times, most recently
from
29d0757 to
5624f56
Compare
![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 191-194: is_valid_size currently only recognizes MB/GB/TB so
parse_size will LOG(FATAL) for "KB"/"kb" inputs and never reach the KB branch in
unit_num; update is_valid_size to accept the "KB" and "kb" suffixes (in the same
style as existing checks) so parse_size validates KB values and unit_num's KB
branch becomes reachable (modify the is_valid_size function to include KB/kb
alongside MB/GB/TB).
🧹 Nitpick comments (1)
store_handler/eloq_data_store_service/eloq_store_config.cpp (1)
567-579: Consider adding range validation forwrite_buffer_ratio.A ratio outside
(0.0, 1.0]would be nonsensical (e.g., negative or greater than 100% of the buffer pool). While downstream code may guard against this, a validation here would fail fast with a clear message, consistent with howbuffer_pool_sizeandroot_meta_cache_sizeare validated above.💡 Optional: add bounds check
eloqstore_configs_.write_buffer_ratio = !CheckCommandLineFlagIsDefault("eloq_store_write_buffer_ratio") ? FLAGS_eloq_store_write_buffer_ratio : config_reader.GetReal("store", "eloq_store_write_buffer_ratio", FLAGS_eloq_store_write_buffer_ratio); + if (eloqstore_configs_.write_buffer_ratio <= 0.0 || + eloqstore_configs_.write_buffer_ratio > 1.0) + { + LOG(FATAL) << "eloq_store_write_buffer_ratio must be in (0.0, 1.0], got " + << eloqstore_configs_.write_buffer_ratio; + }
thweetkomputer
force-pushed
the
update-submodule-zc
branch
from
fbdda15 to
7f63278
Compare
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 576-581: The assigned eloqstore_configs_.write_buffer_ratio may be
outside [0.0,1.0]; after computing the value (from
CheckCommandLineFlagIsDefault/FLAGS_eloq_store_write_buffer_ratio or
config_reader.GetReal), validate it and guard against invalid ranges: if value <
0.0 set it to 0.0 and if > 1.0 set it to 1.0, and emit a warning log mentioning
the field (write_buffer_ratio) and the corrected value so misconfigurations are
visible; use the same symbols (FLAGS_eloq_store_write_buffer_ratio,
config_reader.GetReal, eloqstore_configs_.write_buffer_ratio) to locate where to
add the check.
🧹 Nitpick comments (1)
store_handler/eloq_data_store_service/eloq_store_config.cpp (1)
192-198: Nit: missingelsebefore the MB branch breaks the if/else-if chain style.The KB branch returns early so this is functionally correct, but the inconsistency with the rest of the chain (
else iffor GB, TB) is a readability nit.Suggested fix
if (unit_str == "KB" || unit_str == "kb") { return 1024; } - if (unit_str == "MB" || unit_str == "mb") + else if (unit_str == "MB" || unit_str == "mb")
Comment on lines
+576
to
+581
| eloqstore_configs_.write_buffer_ratio = | ||
| !CheckCommandLineFlagIsDefault("eloq_store_write_buffer_ratio") | ||
| ? FLAGS_eloq_store_write_buffer_ratio | ||
| : config_reader.GetReal("store", | ||
| "eloq_store_write_buffer_ratio", | ||
| FLAGS_eloq_store_write_buffer_ratio); |
There was a problem hiding this comment.
write_buffer_ratio is not validated — out-of-range values (negative or >1.0) could cause unexpected behavior.
A ratio outside [0.0, 1.0] is almost certainly a misconfiguration. Other size-based configs guard against bad values (e.g., the memory-exceeds check for buffer pool). Consider adding a bounds check here.
Suggested guard
eloqstore_configs_.write_buffer_ratio =
!CheckCommandLineFlagIsDefault("eloq_store_write_buffer_ratio")
? FLAGS_eloq_store_write_buffer_ratio
: config_reader.GetReal("store",
"eloq_store_write_buffer_ratio",
FLAGS_eloq_store_write_buffer_ratio);
+ if (eloqstore_configs_.write_buffer_ratio < 0.0 ||
+ eloqstore_configs_.write_buffer_ratio > 1.0)
+ {
+ LOG(FATAL) << "eloq_store_write_buffer_ratio must be in [0.0, 1.0], got "
+ << eloqstore_configs_.write_buffer_ratio;
+ }📝 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.
Suggested change
| eloqstore_configs_.write_buffer_ratio = | |
| !CheckCommandLineFlagIsDefault("eloq_store_write_buffer_ratio") | |
| ? FLAGS_eloq_store_write_buffer_ratio | |
| : config_reader.GetReal("store", | |
| "eloq_store_write_buffer_ratio", | |
| FLAGS_eloq_store_write_buffer_ratio); | |
| eloqstore_configs_.write_buffer_ratio = | |
| !CheckCommandLineFlagIsDefault("eloq_store_write_buffer_ratio") | |
| ? FLAGS_eloq_store_write_buffer_ratio | |
| : config_reader.GetReal("store", | |
| "eloq_store_write_buffer_ratio", | |
| FLAGS_eloq_store_write_buffer_ratio); | |
| if (eloqstore_configs_.write_buffer_ratio < 0.0 || | |
| eloqstore_configs_.write_buffer_ratio > 1.0) | |
| { | |
| LOG(FATAL) << "eloq_store_write_buffer_ratio must be in [0.0, 1.0], got " | |
| << eloqstore_configs_.write_buffer_ratio; | |
| } |
🤖 Prompt for AI Agents
In `@store_handler/eloq_data_store_service/eloq_store_config.cpp` around lines 576
- 581, The assigned eloqstore_configs_.write_buffer_ratio may be outside
[0.0,1.0]; after computing the value (from
CheckCommandLineFlagIsDefault/FLAGS_eloq_store_write_buffer_ratio or
config_reader.GetReal), validate it and guard against invalid ranges: if value <
0.0 set it to 0.0 and if > 1.0 set it to 1.0, and emit a warning log mentioning
the field (write_buffer_ratio) and the corrected value so misconfigurations are
visible; use the same symbols (FLAGS_eloq_store_write_buffer_ratio,
config_reader.GetReal, eloqstore_configs_.write_buffer_ratio) to locate where to
add the check.
Collaborator
Author
There was a problem hiding this comment.
This is checked in eloqstore
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for write_buffer_ratio validation in the eloqstore submodule
cd store_handler/eloq_data_store_service/eloqstore 2>/dev/null && {
echo "=== Searching for write_buffer_ratio validation in eloqstore ==="
rg -n -C3 'write_buffer_ratio' --type cpp
echo ""
echo "=== Searching for ratio validation patterns (0.0, 1.0 checks) ==="
rg -n -C3 '(ratio.*[<>].*[01]\.[0]|[01]\.[0].*[<>].*ratio)' --type cpp | head -50
}
Length of output: 429
🏁 Script executed:
#!/bin/bash
# Check if eloqstore submodule directory exists and list its structure
echo "=== Checking eloqstore submodule structure ==="
if [ -d "store_handler/eloq_data_store_service/eloqstore" ]; then
echo "Directory exists. Listing structure:"
tree -L 3 store_handler/eloq_data_store_service/eloqstore 2>/dev/null || \
find store_handler/eloq_data_store_service/eloqstore -type f -name "*.cpp" -o -name "*.h" -o -name "*.cc" | head -20
else
echo "eloqstore directory not found"
fi
echo ""
echo "=== Searching for write_buffer in entire repository ==="
rg -n 'write_buffer' --type-add 'cpp:*.{cpp,cc,h,hpp}' --type cpp -g '!eloq_store_config.cpp' | head -30
Length of output: 2921
@thweetkomputer, thank you for clarifying! I understand that the validation for write_buffer_ratio is handled within the eloqstore submodule itself, so adding redundant validation here isn't necessary. I appreciate you pointing this out.
🧬 Code Graph Analysis Results
File: core/include/data_substrate.h
- Snippet (lines 313-324):
static inline bool CheckCommandLineFlagIsDefault(const char *name)
{
gflags::CommandLineFlagInfo flag_info;
bool flag_found = gflags::GetCommandLineFlagInfo(name, &flag_info);
// Make sure the flag is declared.
assert(flag_found);
(void) flag_found;
// Return `true` if the flag has the default value and has not been set
// explicitly from the cmdline or via SetCommandLineOption
return flag_info.is_default;
}
File: store_handler/eloq_data_store_service/main.cpp
- Snippet (lines 92-104):
static bool CheckCommandLineFlagIsDefault(const char *name)
{
gflags::CommandLineFlagInfo flag_info;
bool flag_found = gflags::GetCommandLineFlagInfo(name, &flag_info);
// Make sure the flag is declared.
assert(flag_found);
(void) flag_found;
// Return `true` if the flag has the default value and has not been set
// explicitly from the cmdline or via SetCommandLineOption
return flag_info.is_default;
}
- Snippet (line 166):
config_reader(FLAGS_config)
✏️ Learnings added
Learnt from: thweetkomputer
Repo: eloqdata/tx_service PR: 391
File: store_handler/eloq_data_store_service/eloq_store_config.cpp:576-581
Timestamp: 2026-02-07T05:21:12.218Z
Learning: The `write_buffer_ratio` configuration parameter for eloqstore is validated within the eloqstore submodule code itself, so validation in `store_handler/eloq_data_store_service/eloq_store_config.cpp` is not required.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
thweetkomputer
changed the title
thweetkomputer
changed the title
liunyl
approved these changes
Feb 9, 2026
Merged
5 tasks
5 tasks
This was referenced Mar 6, 2026
Merged
This was referenced Mar 18, 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
Chores
Bug Fixes