feat: adapt to eloqstore memory upload and add eloq_store_max_write_concurrency parameters#407
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds new eloq_store_max_write_concurrency flag and INI wiring, extends EloqStoreConfig constructor to accept core_number, introduces cloud_request_threads computation (flag → INI → derived from core_number) and core-reservation logic that can reduce core_num and update runtime concurrency, and updates an eloqstore submodule reference and constructor call sites. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@core/src/storage_init.cpp`:
- Around line 292-306: The conditional that compares original_core_num to
core_config_.core_num is dead code because EloqStoreConfig's constructor does
not modify core_config_.core_num; either implement the adjustment logic inside
EloqStoreConfig (e.g., update core_config_.core_num within the
EloqDS::EloqStoreConfig constructor when reserving threads for the cloud) so the
post-construction check can detect changes, or remove the entire block that
checks original_core_num and calls GFLAGS_NAMESPACE::SetCommandLineOption for
"bthread_concurrency" and "eloq_store_worker_num" to avoid confusing dead
branches; refer to EloqStoreConfig, core_config_.core_num, original_core_num,
bthread_concurrency, and eloq_store_worker_num to find the relevant code.
In `@store_handler/eloq_data_store_service/eloq_store_config.cpp`:
- Around line 267-272: The constructor EloqStoreConfig currently accepts
core_number_auto_config and a non-const core_number reference but never uses
them; either remove these parameters from the signature and update
declarations/usages (constructor in header and all call sites) or implement the
auto-config behavior: if core_number_auto_config is true compute the desired
core count (e.g., using hardware_concurrency or INIReader values), set
core_number (the out-parameter) and assign that value to core_config_.core_num
inside the EloqStoreConfig constructor so the caller can observe changes; ensure
the header signature, all call sites, and any related logic that reads
core_config_.core_num are updated consistently.
🧹 Nitpick comments (4)
core/include/data_substrate.h (1)
128-141: Inconsistent naming: trailing underscore oncore_num_auto_config_differs from sibling fields.All other members of
CoreConfig(e.g.,core_num,enable_heap_defragment,enable_wal) omit the trailing underscore. Consider renaming tocore_num_auto_configfor consistency.store_handler/eloq_data_store_service/eloq_store_config.cpp (1)
609-631: Duplicatecloud_request_threadscomputation withdata_substrate.cpp.The logic here (lines 609–631) to derive
cloud_request_threadsfrom flag → INI →core_number / 4is nearly identical to the block incore/src/data_substrate.cpp(lines 732–791). The data_substrate version additionally reserves cores by reducingcore_num, while this version only setscloud_request_threadson the config struct.This duplication means the derivation logic must be kept in sync in two places. Consider consolidating so the thread count is computed once and passed in, or having
EloqStoreConfigbe the single owner of this logic.Also note: lines 597–602 already unconditionally set
cloud_request_threads(via the standard flag/INI pattern), and then lines 609–631 immediately overwrite it whencloud_store_pathis non-empty. The initial assignment on line 597 is wasted work in that case.store_handler/eloq_data_store_service/main.cpp (1)
33-33:#include <thread>appears unused in this file.No
std::threadorstd::thread::hardware_concurrency()usage is visible. If it was added only for the constructor signature change, it's not needed here.core/src/data_substrate.cpp (1)
729-791: Core reservation logic is safe at boundary conditions but duplicateseloq_store_config.cpp.The
core_num > 1guard on line 766 correctly prevents reducingcore_numto zero, and thestd::minon line 770–771 ensures the decrease never exceedscore_num - 1. The boundary behavior for small VCPU counts (1–3) is sound.However, the cloud_request_threads derivation logic (flag → INI →
core_num / 4) is duplicated between here andeloq_store_config.cpplines 609–631. As noted in that file's review, consider consolidating.Also note: on line 768,
std::max<size_t>(1, cloud_request_threads)is redundant sincecloud_request_threads > 0is already guaranteed by the enclosingifon line 765.
thweetkomputer
force-pushed
the
eloqstore-memory-upload
branch
from
33e08b4 to
a25afaa
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 267-271: The constructor parameter uint32_t &core_number in
EloqStoreConfig::EloqStoreConfig is a non-const reference but never modified,
which misleads callers (and storage_init logic expects potential write-back);
either change the signature to const uint32_t& or uint32_t (pass-by-value) and
remove the dependent dead-code in storage_init, or add proper write-back inside
EloqStoreConfig::EloqStoreConfig to update the referenced core_number with the
CPU reservation decision made by the constructor so storage_init's later checks
observe the modified value; update callers/tests accordingly to match the chosen
approach.
🧹 Nitpick comments (3)
core/src/data_substrate.cpp (2)
730-787: Cloud thread reservation only applies whencore_numberis auto-configured.This entire block lives inside the
elsebranch (line 719) that only executes whencore_numberis not set via flag or INI. If a user explicitly setscore_numberin the config but relies on defaultcloud_request_threads, no CPU reservation will occur. Is this intentional?Additionally, the
SetCommandLineOptioncalls on lines 777–782 are redundant — the same flags are unconditionally set tocore_config_.core_numlater at lines 877–878 and 889–890, andcore_config_.core_numalready holds the decreased value by then.♻️ Remove redundant early SetCommandLineOption calls
if (decrease > 0) { core_config_.core_num -= decrease; LOG(INFO) << "decrease core_number to " << core_config_.core_num << " to reserve cpu for eloqstore " "threads."; - auto adjusted_core_num = - std::to_string(core_config_.core_num); - GFLAGS_NAMESPACE::SetCommandLineOption( - "bthread_concurrency", - adjusted_core_num.c_str()); - GFLAGS_NAMESPACE::SetCommandLineOption( - "eloq_store_worker_num", - adjusted_core_num.c_str()); }Please confirm whether CPU reservation should also apply when
core_numberis explicitly set (via flag or INI) with cloud store enabled, or if the current auto-config-only behavior is by design.
762-767:reserved_cpuis always equal tocloud_request_threadshere.On line 762, the condition already guarantees
cloud_request_threads > 0, sostd::max<size_t>(1, cloud_request_threads)on line 764–765 is a no-op. Consider simplifying.♻️ Simplify
- if (cloud_request_threads > 0 && core_config_.core_num > 1) - { - auto reserved_cpu = - std::max<size_t>(1, cloud_request_threads); - auto decrease = std::min<size_t>( - reserved_cpu, core_config_.core_num - 1); + if (cloud_request_threads > 0 && core_config_.core_num > 1) + { + auto decrease = std::min<size_t>( + cloud_request_threads, core_config_.core_num - 1);store_handler/eloq_data_store_service/eloq_store_config.cpp (1)
608-630:cloud_request_threadsis set twice — second block fully overrides the first when cloud path is non-empty.Lines 596–601 unconditionally set
cloud_request_threads(flag → INI → flag-default). Then lines 608–630 completely re-determine it whencloud_store_pathis non-empty. The only semantic difference is the fallback:1(flag default) vsmax(1, core_number / 4).Consider restructuring so the cloud-path-specific default is applied in a single pass to avoid confusion.
Also, the INI fallback on line 617 is
0(viaGetInteger(..., 0)) whereas the earlier read on line 601 falls back toFLAGS_eloq_store_cloud_request_threads(which is1). If the INI key exists but holds an unparseable value, this inconsistency would yield different defaults depending on which code path ran last.♻️ Consolidate cloud_request_threads logic
eloqstore_configs_.cloud_request_threads = !CheckCommandLineFlagIsDefault("eloq_store_cloud_request_threads") ? FLAGS_eloq_store_cloud_request_threads : config_reader.GetInteger("store", "eloq_store_cloud_request_threads", FLAGS_eloq_store_cloud_request_threads); + // When cloud store is enabled and no explicit value was provided, + // scale cloud_request_threads with core count instead of using flag default. + if (!eloqstore_configs_.cloud_store_path.empty() && + CheckCommandLineFlagIsDefault("eloq_store_cloud_request_threads") && + !config_reader.HasValue("store", "eloq_store_cloud_request_threads")) + { + eloqstore_configs_.cloud_request_threads = + std::max<size_t>(1, core_number / 4); + } - if (!eloqstore_configs_.cloud_store_path.empty()) - { - if (CheckCommandLineFlagIsDefault("eloq_store_cloud_request_threads")) - { - if (config_reader.HasValue("store", - "eloq_store_cloud_request_threads")) - { - eloqstore_configs_.cloud_request_threads = - config_reader.GetInteger( - "store", "eloq_store_cloud_request_threads", 0); - } - else - { - eloqstore_configs_.cloud_request_threads = - std::max<size_t>(1, core_number / 4); - } - } - else - { - eloqstore_configs_.cloud_request_threads = - FLAGS_eloq_store_cloud_request_threads; - } - }
2 participants
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