Copilot commented on code in PR #3547:
URL: https://github.com/apache/brpc/pull/3547#discussion_r4022137655
##########
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.
T result = _counter.value();
_counter.reset();
return result;
Review Comment:
The new Babylon reset path is not exercised: the reducer tests cover
`Adder::get_value()` but never assert that `reset()` returns the accumulated
value and restores the identity. This path replaces the previous inverted
CHECK, so add a test that runs with `WITH_BABYLON_COUNTER` enabled as well as
with the generic backend.
--
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]
