Conversation
- Add optional `thread_config` support for the executor update thread - Log cumulative and 5s window timing stats for start lateness, update duration, skipped cycles, and top component costs - Vendor a local `TDigest` utility for percentile reporting - Switch affected packages to C++23 for `std::format` and `std::expected` - Allow starting the executor with an empty `components` list for testing
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Walkthrough本次改动将 ChangesExecutor 线程调度与统计
TDigest 与 ThreadConfig 工具类新增
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 5
🧹 Nitpick comments (1)
rmcs_ws/src/rmcs_utility/include/rmcs_utility/thread_config.hpp (1)
83-112: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win改用线程安全的错误信息格式化
这 4 处都在用
std::strerror，并发时可能串用同一个静态缓冲区；建议统一改成std::system_category().message(error_code)，errno分支也改用同样的方式（可补#include <system_error>）。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rmcs_ws/src/rmcs_utility/include/rmcs_utility/thread_config.hpp` around lines 83 - 112, The error formatting in ThreadConfig::apply uses std::strerror in multiple failure paths, which is not thread-safe and can return mixed messages under concurrency. Update the scheduling, nice, affinity, and name error branches in thread_config.hpp to format messages via std::system_category().message(error_code) instead of std::strerror, and apply the same approach to the errno-based setpriority path; add the needed system_error include if it is not already present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rmcs_ws/src/rmcs_executor/src/executor.hpp`:
- Around line 125-126: The update_rate to period conversion in executor.hpp is
not validated, so non-finite, non-positive, or too-large values can produce an
invalid or zero period and break later logic. In start() of executor::Executor,
validate update_rate with std::isfinite(update_rate) and update_rate > 0.0
before computing the period, then verify the derived std::chrono::nanoseconds
period has a count greater than zero before using it; apply the check around the
existing period calculation and any code that relies on period.count().
In `@rmcs_ws/src/rmcs_utility/include/rmcs_utility/tdigest.hpp`:
- Around line 158-168: `TDigest::insert` only updates `buffer` until `merge()`
is triggered, but the read paths (`quantile`, `cumulative_distribution`, `get`,
`size`, and the executor’s `maybe_quantile` call site) appear to read only from
`active`, so pending samples can be invisible. Update the `TDigest` read API to
include buffered samples, either by flushing a snapshot before queries or by
merging/considering `buffer` inside the query methods, and make the `executor`
path use the corrected `TDigest::quantile` behavior so `sample_count > 0` never
reports stale or empty results.
- Around line 256-264: Reject zero-sized TDigest construction in
TDigest::TDigest(size_t size) by validating size at the start of the constructor
and preventing creation when size == 0. The issue is that one/two and the active
digest end up with zero capacity, which later breaks merge normalization in
merge() via normalizer_fn on inactive.values.capacity(). Add an upfront guard in
the TDigest constructor to fail fast with a clear error or exception so callers
cannot instantiate TDigest(0).
- Around line 305-318: The TDigest::insert(const TDigest& src) implementation
only copies src.active->values and ignores any pending samples still sitting in
src.buffer, so merge the source digest state first (on a local copy) before
inserting its centroids into this digest. Update TDigest::insert to work from a
merged copy of src, then feed that copy’s active values through the existing
buffer/merge flow so max_val/min_val and the actual data stay consistent.
- Around line 41-47: The tdigest.hpp header uses std::pair and std::make_pair
without explicitly including the required <utility> header, relying on indirect
includes. Update the header’s include list to add <utility> so the template code
in tdigest.hpp remains self-contained and compiles independently.
---
Nitpick comments:
In `@rmcs_ws/src/rmcs_utility/include/rmcs_utility/thread_config.hpp`:
- Around line 83-112: The error formatting in ThreadConfig::apply uses
std::strerror in multiple failure paths, which is not thread-safe and can return
mixed messages under concurrency. Update the scheduling, nice, affinity, and
name error branches in thread_config.hpp to format messages via
std::system_category().message(error_code) instead of std::strerror, and apply
the same approach to the errno-based setpriority path; add the needed
system_error include if it is not already present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 794a3821-8ac4-4416-b7e2-018af9763e6c
📒 Files selected for processing (6)
rmcs_ws/src/rmcs_executor/CMakeLists.txtrmcs_ws/src/rmcs_executor/src/executor.hpprmcs_ws/src/rmcs_executor/src/main.cpprmcs_ws/src/rmcs_utility/CMakeLists.txtrmcs_ws/src/rmcs_utility/include/rmcs_utility/tdigest.hpprmcs_ws/src/rmcs_utility/include/rmcs_utility/thread_config.hpp
qzhhhi
added a commit
that referenced
this pull request
Jul 7, 2026
- Add optional `thread_config` support for the executor update thread - Log cumulative and 5s window timing stats for start lateness, update duration, skipped cycles, and top component costs - Vendor a local `TDigest` utility for percentile reporting - Switch affected packages to C++23 for `std::format` and `std::expected` - Allow starting the executor with an empty `components` list for testing
creeper5820
pushed a commit
that referenced
this pull request
Jul 8, 2026
- Add optional `thread_config` support for the executor update thread - Log cumulative and 5s window timing stats for start lateness, update duration, skipped cycles, and top component costs - Vendor a local `TDigest` utility for percentile reporting - Switch affected packages to C++23 for `std::format` and `std::expected` - Allow starting the executor with an empty `components` list for testing
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.
thread_configsupport for the executor update threadTDigestutility for percentile reportingstd::formatandstd::expectedcomponentslist for testing主要变更
thread_config支持：可通过配置解析并应用线程名、CPU 亲和性、调度策略（policy）、优先级（priority）与 nice（nice），并在配置校验与应用失败时返回错误结果。TDigest实现，用于分位数（quantile）与累计分布统计。CMAKE_CXX_STANDARD 23，以便使用std::format、std::expected等能力。components列表启动执行器；同时调整main()对components参数读取逻辑的处理方式（不再基于布尔返回值抛出显式异常）。其他
rmcs_utility::ThreadConfig（线程配置解析与应用）、rmcs_utility::TDigest（分位数统计）。