chenBright commented on code in PR #3547:
URL: https://github.com/apache/brpc/pull/3547#discussion_r4022015641
##########
src/bvar/reducer.h:
##########
@@ -93,17 +121,18 @@ class BabylonVariable: public Variable {
}
T get_value() const {
+ CHECK(!(butil::is_same<InvOp, VoidOp>::value) || nullptr == _sampler)
+ << "You should not call Reducer<" << butil::class_name_str<T>()
+ << ", " << butil::class_name_str<Op>() << ">::get_value() when a"
+ << " Window<> is used because the operator does not have inverse.";
return _counter.value();
}
T reset() {
- if (BAIDU_UNLIKELY((!butil::is_same<VoidOp, InvOp>::value))) {
- CHECK(false) << "You should not call Reducer<" <<
butil::class_name_str<T>()
- << ", " << butil::class_name_str<Op>() <<
">::get_value() when a"
- << " Window<> is used because the operator does not
have inverse.";
- return get_value();
- }
-
+ // Unlike AgentCombiner::reset_all_agents(), reading and clearing the
babylon
+ // counter are two separate steps, so values added in between are
lost. This
+ // only affects explicit reset() by users: sampling of an operator
without
+ // inverse is the only internal user and it runs in a single thread.
Review Comment:
This is a design limitation of the babylon counters, expected and acceptable
here.
babylon offers no atomic read-and-clear: `value()` aggregates the
thread-local
slots and `reset()` only bumps a version counter. babylon documents the very
same
window on the write side (`counter.h`, `operator<<`): a sample landing
across a
version bump is attributed to the previous period and dropped, and the author
explicitly deems it negligible for statistics. Providing a
snapshot-and-clear path
would mean synchronizing every `operator<<`, which is the cost the babylon
backend
exists to avoid.
The only internal caller is `ReducerSampler::take_sample()`, which for an
operator
without inverse runs in the single sampling thread, so sampling is
unaffected. Only
an explicit user `reset()` racing with `operator<<` can lose a sample, which
is
what the comment at that line states.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
