change worker_polling_time_us to 100000 and default core_number#342
Conversation
WalkthroughReplaced conditional per-branch core_num logic with a single automatic calculation and increased BRPC worker polling interval from 1,000 µs to 100,000 µs (in core/src/data_substrate.cpp). Added a new EloqStore configuration flag and wiring for per-core max concurrent writes (in eloq_store_config.cpp). Changes
Sequence Diagram(s)(omitted — changes are configuration and local control-flow adjustments that do not introduce a new multi-component sequential flow) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (3 warnings)
✏️ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @core/src/data_substrate.cpp:
- Line 797: You changed the BRPC polling interval by calling
GFLAGS_NAMESPACE::SetCommandLineOption("worker_polling_time_us", "100000");
which needs validation and documentation; run comparative performance tests
(measure request latency including tail percentiles and CPU utilization) against
the default polling value, add a short rationale document explaining why 100ms
suits the ELOQ workload (trade-offs of CPU vs latency), add automated tests that
exercise the code path with both default and 100ms settings (asserting
latency/throughput bounds), and record rpcz/bvar profiling output to verify
behavior; update the PR to include the test additions, the performance results
artifact, the rationale text, and mention the exact flag name
worker_polling_time_us and the SetCommandLineOption call so reviewers can locate
the change.
4e593b2 to
07b909b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/data_substrate.cpp (1)
724-727: Significant increase in automatic core allocation requires documentation.The core_num formula changed from conditional 60-70% to a fixed 90% of vCPUs. This is a substantial increase that could affect:
- Memory pressure from additional bthreads
- CPU contention with other system processes
- Resource behavior on systems with data store disabled (previously used 70%, now 90%)
Please document the rationale for this change and verify performance under various deployment scenarios.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/data_substrate.cppstore_handler/eloq_data_store_service/eloq_store_config.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
store_handler/eloq_data_store_service/eloq_store_config.cpp (2)
core/include/data_substrate.h (1)
CheckCommandLineFlagIsDefault(313-324)store_handler/eloq_data_store_service/main.cpp (3)
CheckCommandLineFlagIsDefault(89-101)CheckCommandLineFlagIsDefault(89-89)config_reader(163-163)
🔇 Additional comments (2)
store_handler/eloq_data_store_service/eloq_store_config.cpp (2)
127-129: LGTM!The new flag follows the established pattern and the default value of 1 is appropriately conservative for write concurrency control.
515-520: LGTM!The configuration reading follows the established pattern used throughout this file, correctly prioritizing command-line flags over INI config values.
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
New Features
✏️ Tip: You can customize this high-level summary in your review settings.