Copilot commented on code in PR #3532:
URL: https://github.com/apache/brpc/pull/3532#discussion_r3958839266
##########
test/brpc_circuit_breaker_unittest.cpp:
##########
@@ -150,6 +151,60 @@ TEST_F(CircuitBreakerTest, should_not_isolate) {
}
}
+TEST_F(CircuitBreakerTest, canceled_requests_during_initialization) {
+ brpc::CircuitBreaker baseline;
+ for (int i = 0; i < 2 * kLongWindowSize; ++i) {
+ ASSERT_TRUE(_circuit_breaker.OnCallEnd(ECANCELED, kLatency));
+ }
+ EXPECT_EQ(0, _circuit_breaker.isolated_times());
+
+ // Cancellations must not advance initialization or consume its error
budget.
+ bool healthy = true;
+ for (int i = 0; i < kLongWindowSize && healthy; ++i) {
+ healthy = baseline.OnCallEnd(kErrorCodeForFailed, kErrorCost);
+ ASSERT_EQ(healthy,
+ _circuit_breaker.OnCallEnd(kErrorCodeForFailed, kErrorCost));
+ }
+ EXPECT_FALSE(healthy);
+ EXPECT_EQ(1, _circuit_breaker.isolated_times());
+}
Review Comment:
The test intends to verify that cancellations do not advance initialization,
but it never applies cancellations to `baseline` (only to `_circuit_breaker`).
With this setup, `baseline` and `_circuit_breaker` have intentionally different
histories before the failure loop, so `ASSERT_EQ(healthy,
_circuit_breaker.OnCallEnd(...))` is not a reliable assertion of ‘no
initialization advancement’. Consider also calling
`baseline.OnCallEnd(ECANCELED, kLatency)` in the first loop (or remove the
baseline comparison and assert directly on breaker state/transition points) so
both breakers experience identical cancellation sequences before comparing
behavior.
##########
test/brpc_circuit_breaker_unittest.cpp:
##########
@@ -150,6 +151,60 @@ TEST_F(CircuitBreakerTest, should_not_isolate) {
}
}
+TEST_F(CircuitBreakerTest, canceled_requests_during_initialization) {
+ brpc::CircuitBreaker baseline;
+ for (int i = 0; i < 2 * kLongWindowSize; ++i) {
+ ASSERT_TRUE(_circuit_breaker.OnCallEnd(ECANCELED, kLatency));
+ }
+ EXPECT_EQ(0, _circuit_breaker.isolated_times());
+
+ // Cancellations must not advance initialization or consume its error
budget.
+ bool healthy = true;
+ for (int i = 0; i < kLongWindowSize && healthy; ++i) {
+ healthy = baseline.OnCallEnd(kErrorCodeForFailed, kErrorCost);
+ ASSERT_EQ(healthy,
+ _circuit_breaker.OnCallEnd(kErrorCodeForFailed, kErrorCost));
+ }
+ EXPECT_FALSE(healthy);
+ EXPECT_EQ(1, _circuit_breaker.isolated_times());
+}
+
+TEST_F(CircuitBreakerTest, canceled_requests_after_initialization) {
+ brpc::CircuitBreaker baseline;
+ for (int i = 0; i < 2 * kLongWindowSize; ++i) {
+ ASSERT_TRUE(baseline.OnCallEnd(0, kLatency));
+ ASSERT_TRUE(_circuit_breaker.OnCallEnd(0, kLatency));
+ }
+
+ // Interleaved cancellations must neither add error cost nor decay it
+ // like successful requests, regardless of their latency.
+ bool healthy = true;
+ for (int i = 0; i < 2 * kLongWindowSize && healthy; ++i) {
+ ASSERT_TRUE(_circuit_breaker.OnCallEnd(ECANCELED, 1));
+ ASSERT_TRUE(_circuit_breaker.OnCallEnd(ECANCELED, 100 * kLatency));
+ healthy = baseline.OnCallEnd(kErrorCodeForFailed, kErrorCost);
+ ASSERT_EQ(healthy,
+ _circuit_breaker.OnCallEnd(kErrorCodeForFailed, kErrorCost));
+ }
+ EXPECT_FALSE(healthy);
+ EXPECT_EQ(1, _circuit_breaker.isolated_times());
+}
Review Comment:
This test uses a `baseline` breaker for comparison but only applies
cancellations to `_circuit_breaker`, which makes the equality assertion
dependent on internal sampling/window details and less clear about what
invariant is being proven. To make the intent unambiguous, consider either (a)
applying the same ECANCELED calls to `baseline` and asserting the two breakers
behave identically, or (b) dropping the baseline and asserting directly that
ECANCELED does not change isolation behavior (e.g., isolate occurs after the
same number of real errors as in a pure-error sequence).
--
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]
