chenBright commented on code in PR #3547:
URL: https://github.com/apache/brpc/pull/3547#discussion_r4045623202
##########
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:
> If this is an accepted trade-off for the Babylon backend, could we update
the comment to acknowledge that it > also applies to normal sampling?
@wasphin You are right. Updated the comment to explicitly state that this
affects sampler-thread sampling (e.g. Window/series sampling) as well, and that
it's an accepted trade-off for the babylon backend since minor loss doesn't
change the overall trend in statistics/monitoring scenarios.
--
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]
