Copilot commented on code in PR #3219: URL: https://github.com/apache/brpc/pull/3219#discussion_r2782261739
########## test/brpc_auto_concurrency_limiter_unittest.cpp: ########## @@ -0,0 +1,169 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include "brpc/policy/auto_concurrency_limiter.h" +#include "butil/time.h" +#include "bthread/bthread.h" +#include <gtest/gtest.h> + +namespace brpc { +namespace policy { + +DECLARE_int32(auto_cl_sample_window_size_ms); +DECLARE_int32(auto_cl_min_sample_count); +DECLARE_int32(auto_cl_max_sample_count); +DECLARE_bool(auto_cl_enable_error_punish); +DECLARE_double(auto_cl_fail_punish_ratio); +DECLARE_double(auto_cl_error_rate_punish_threshold); + +} // namespace policy +} // namespace brpc + +class AutoConcurrencyLimiterTest : public ::testing::Test { +protected: + void SetUp() override { + // Save original values + orig_sample_window_size_ms_ = brpc::policy::FLAGS_auto_cl_sample_window_size_ms; + orig_min_sample_count_ = brpc::policy::FLAGS_auto_cl_min_sample_count; + orig_max_sample_count_ = brpc::policy::FLAGS_auto_cl_max_sample_count; + orig_enable_error_punish_ = brpc::policy::FLAGS_auto_cl_enable_error_punish; + orig_fail_punish_ratio_ = brpc::policy::FLAGS_auto_cl_fail_punish_ratio; + orig_error_rate_threshold_ = brpc::policy::FLAGS_auto_cl_error_rate_punish_threshold; + + // Set test-friendly values + brpc::policy::FLAGS_auto_cl_sample_window_size_ms = 1000; + brpc::policy::FLAGS_auto_cl_min_sample_count = 5; + brpc::policy::FLAGS_auto_cl_max_sample_count = 200; + brpc::policy::FLAGS_auto_cl_enable_error_punish = true; + brpc::policy::FLAGS_auto_cl_fail_punish_ratio = 1.0; + } + + void TearDown() override { + // Restore original values + brpc::policy::FLAGS_auto_cl_sample_window_size_ms = orig_sample_window_size_ms_; + brpc::policy::FLAGS_auto_cl_min_sample_count = orig_min_sample_count_; + brpc::policy::FLAGS_auto_cl_max_sample_count = orig_max_sample_count_; + brpc::policy::FLAGS_auto_cl_enable_error_punish = orig_enable_error_punish_; + brpc::policy::FLAGS_auto_cl_fail_punish_ratio = orig_fail_punish_ratio_; + brpc::policy::FLAGS_auto_cl_error_rate_punish_threshold = orig_error_rate_threshold_; + } + +private: + int32_t orig_sample_window_size_ms_; + int32_t orig_min_sample_count_; + int32_t orig_max_sample_count_; + bool orig_enable_error_punish_; + double orig_fail_punish_ratio_; + double orig_error_rate_threshold_; +}; + +// Helper function to add samples and trigger window completion +// Uses synthetic timestamps instead of sleeping for faster, deterministic tests. +// The final successful sample is used as the trigger, so actual counts match +// succ_count/fail_count exactly (preserving intended error rates). +void AddSamplesAndTriggerWindow(brpc::policy::AutoConcurrencyLimiter& limiter, + int succ_count, int64_t succ_latency, + int fail_count, int64_t fail_latency) { + ASSERT_GT(succ_count, 0) << "Need at least 1 success to trigger window"; + int64_t now = butil::gettimeofday_us(); + + // Add successful samples (reserve one for the trigger) + for (int i = 0; i < succ_count - 1; ++i) { + limiter.AddSample(0, succ_latency, now); + } + // Add failed samples + for (int i = 0; i < fail_count; ++i) { + limiter.AddSample(1, fail_latency, now); + } + + // Advance timestamp past window expiry instead of sleeping + int64_t after_window = now + brpc::policy::FLAGS_auto_cl_sample_window_size_ms * 1000 + 1000; + + // Use the final success sample to trigger window submission + limiter.AddSample(0, succ_latency, after_window); +} + +// Test 1: Backward compatibility - threshold=0 preserves original punishment behavior +TEST_F(AutoConcurrencyLimiterTest, ThresholdZeroPreservesOriginalBehavior) { + brpc::policy::FLAGS_auto_cl_error_rate_punish_threshold = 0; + brpc::policy::FLAGS_auto_cl_sample_window_size_ms = 10; + + brpc::policy::AutoConcurrencyLimiter limiter; + AddSamplesAndTriggerWindow(limiter, 90, 100, 10, 1000); + + // 10% error rate, threshold=0 means full punishment applied + // avg_latency = ceil((10*1000 + 90*100) / 90) = ceil(211.1) = 212us + ASSERT_GT(limiter._min_latency_us, 180); + ASSERT_LT(limiter._min_latency_us, 250); Review Comment: This test directly reads `limiter._min_latency_us`, but `_min_latency_us` is a private member of `AutoConcurrencyLimiter`, so the test will not compile. Please access this via a public/testing accessor (e.g., a `min_latency_us_for_test()` under `#ifdef UNIT_TEST` or `FRIEND_TEST`), or assert behavior through a public API instead of private state. ########## test/brpc_auto_concurrency_limiter_unittest.cpp: ########## @@ -0,0 +1,169 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include "brpc/policy/auto_concurrency_limiter.h" +#include "butil/time.h" +#include "bthread/bthread.h" Review Comment: `#include "bthread/bthread.h"` appears unused in this test file. Removing unused includes helps keep build time and dependencies minimal. ```suggestion ``` ########## src/brpc/policy/auto_concurrency_limiter.cpp: ########## @@ -77,6 +77,14 @@ DEFINE_int32(auto_cl_latency_fluctuation_correction_factor, 1, "the value, the higher the tolerance for the fluctuation of the " "latency. If the value is too large, the latency will be higher " "when the server is overloaded."); +DEFINE_double(auto_cl_error_rate_punish_threshold, 0, + "Threshold for error-rate-based punishment attenuation. " + "Valid range: (0, 1). Values outside this range are ignored " + "and original punishment logic is used. " + "0 (default): no effect, original punishment logic is used. " + "e.g. 0.1: error rates below 10%% produce zero punishment; " + "above it the punishment scales linearly from 0 to full strength. " Review Comment: The flag help text says "Valid range: (0, 1)", but `0` is also an explicitly supported value (disable / preserve original behavior). To avoid confusion for operators, consider updating the wording to something like "Valid range: [0, 1) (0 disables)". ########## test/brpc_auto_concurrency_limiter_unittest.cpp: ########## @@ -0,0 +1,169 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include "brpc/policy/auto_concurrency_limiter.h" +#include "butil/time.h" +#include "bthread/bthread.h" +#include <gtest/gtest.h> + +namespace brpc { +namespace policy { + +DECLARE_int32(auto_cl_sample_window_size_ms); +DECLARE_int32(auto_cl_min_sample_count); +DECLARE_int32(auto_cl_max_sample_count); +DECLARE_bool(auto_cl_enable_error_punish); +DECLARE_double(auto_cl_fail_punish_ratio); +DECLARE_double(auto_cl_error_rate_punish_threshold); + +} // namespace policy +} // namespace brpc + +class AutoConcurrencyLimiterTest : public ::testing::Test { +protected: + void SetUp() override { + // Save original values + orig_sample_window_size_ms_ = brpc::policy::FLAGS_auto_cl_sample_window_size_ms; + orig_min_sample_count_ = brpc::policy::FLAGS_auto_cl_min_sample_count; + orig_max_sample_count_ = brpc::policy::FLAGS_auto_cl_max_sample_count; + orig_enable_error_punish_ = brpc::policy::FLAGS_auto_cl_enable_error_punish; + orig_fail_punish_ratio_ = brpc::policy::FLAGS_auto_cl_fail_punish_ratio; + orig_error_rate_threshold_ = brpc::policy::FLAGS_auto_cl_error_rate_punish_threshold; + + // Set test-friendly values + brpc::policy::FLAGS_auto_cl_sample_window_size_ms = 1000; + brpc::policy::FLAGS_auto_cl_min_sample_count = 5; + brpc::policy::FLAGS_auto_cl_max_sample_count = 200; + brpc::policy::FLAGS_auto_cl_enable_error_punish = true; + brpc::policy::FLAGS_auto_cl_fail_punish_ratio = 1.0; + } + + void TearDown() override { + // Restore original values + brpc::policy::FLAGS_auto_cl_sample_window_size_ms = orig_sample_window_size_ms_; + brpc::policy::FLAGS_auto_cl_min_sample_count = orig_min_sample_count_; + brpc::policy::FLAGS_auto_cl_max_sample_count = orig_max_sample_count_; + brpc::policy::FLAGS_auto_cl_enable_error_punish = orig_enable_error_punish_; + brpc::policy::FLAGS_auto_cl_fail_punish_ratio = orig_fail_punish_ratio_; + brpc::policy::FLAGS_auto_cl_error_rate_punish_threshold = orig_error_rate_threshold_; + } + +private: + int32_t orig_sample_window_size_ms_; + int32_t orig_min_sample_count_; + int32_t orig_max_sample_count_; + bool orig_enable_error_punish_; + double orig_fail_punish_ratio_; + double orig_error_rate_threshold_; +}; + +// Helper function to add samples and trigger window completion +// Uses synthetic timestamps instead of sleeping for faster, deterministic tests. +// The final successful sample is used as the trigger, so actual counts match +// succ_count/fail_count exactly (preserving intended error rates). +void AddSamplesAndTriggerWindow(brpc::policy::AutoConcurrencyLimiter& limiter, + int succ_count, int64_t succ_latency, + int fail_count, int64_t fail_latency) { + ASSERT_GT(succ_count, 0) << "Need at least 1 success to trigger window"; + int64_t now = butil::gettimeofday_us(); + + // Add successful samples (reserve one for the trigger) + for (int i = 0; i < succ_count - 1; ++i) { + limiter.AddSample(0, succ_latency, now); + } + // Add failed samples + for (int i = 0; i < fail_count; ++i) { + limiter.AddSample(1, fail_latency, now); + } + + // Advance timestamp past window expiry instead of sleeping + int64_t after_window = now + brpc::policy::FLAGS_auto_cl_sample_window_size_ms * 1000 + 1000; + + // Use the final success sample to trigger window submission + limiter.AddSample(0, succ_latency, after_window); Review Comment: `AutoConcurrencyLimiter::AddSample(...)` is a private method (declared in auto_concurrency_limiter.h), so this helper will not compile as written. Consider driving the limiter via the public `OnResponded(...)` API, or (preferably for deterministic testing) exposing a test-only hook (e.g., `#ifdef UNIT_TEST` method) or adding `FRIEND_TEST` declarations in `AutoConcurrencyLimiter` to allow tests to call `AddSample` with synthetic timestamps. ```suggestion // Drives the limiter via its public OnResponded API instead of the private // AddSample method, to respect encapsulation and keep tests compiling. // The final successful sample is used as the trigger, so actual counts match // succ_count/fail_count exactly (preserving intended error rates). void AddSamplesAndTriggerWindow(brpc::policy::AutoConcurrencyLimiter& limiter, int succ_count, int64_t succ_latency, int fail_count, int64_t fail_latency) { ASSERT_GT(succ_count, 0) << "Need at least 1 success to trigger window"; // Add successful samples (reserve one for the trigger) for (int i = 0; i < succ_count - 1; ++i) { limiter.OnResponded(0, succ_latency); } // Add failed samples for (int i = 0; i < fail_count; ++i) { limiter.OnResponded(1, fail_latency); } // Wait for longer than the sample window so that the limiter will // treat the next sample as belonging to a new window. bthread_usleep(brpc::policy::FLAGS_auto_cl_sample_window_size_ms * 1000 + 1000); // Use the final success sample to trigger window submission limiter.OnResponded(0, succ_latency); ``` -- 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]
