Copilot commented on code in PR #3219: URL: https://github.com/apache/brpc/pull/3219#discussion_r2781390260
########## test/brpc_auto_concurrency_limiter_unittest.cpp: ########## @@ -0,0 +1,208 @@ +// 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 +void AddSamplesAndTriggerWindow(brpc::policy::AutoConcurrencyLimiter& limiter, + int succ_count, int64_t succ_latency, + int fail_count, int64_t fail_latency) { + int64_t now = butil::gettimeofday_us(); + + // Add successful samples + for (int i = 0; i < succ_count; ++i) { + limiter.AddSample(0, succ_latency, now); + } + // Add failed samples + for (int i = 0; i < fail_count; ++i) { + limiter.AddSample(1, fail_latency, now); + } + + // Wait for window to expire and trigger update + bthread_usleep(brpc::policy::FLAGS_auto_cl_sample_window_size_ms * 1000 + 1000); + + // Add one more sample to trigger window submission + limiter.AddSample(0, succ_latency, butil::gettimeofday_us()); Review Comment: `AddSamplesAndTriggerWindow` sleeps to advance the sampling window. Since the test is already calling `AddSample(error_code, latency_us, sampling_time_us)` directly (with an explicit timestamp parameter), consider advancing `sampling_time_us` instead of sleeping. This will make the unit tests faster and less timing-dependent/flaky under loaded CI machines. ```suggestion // Advance sampling time beyond the window size to trigger update, // without actually sleeping. int64_t trigger_time = now + static_cast<int64_t>(brpc::policy::FLAGS_auto_cl_sample_window_size_ms) * 1000 + 1000; // Add one more sample to trigger window submission limiter.AddSample(0, succ_latency, trigger_time); ``` ########## test/brpc_auto_concurrency_limiter_unittest.cpp: ########## @@ -0,0 +1,208 @@ +// 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 +void AddSamplesAndTriggerWindow(brpc::policy::AutoConcurrencyLimiter& limiter, + int succ_count, int64_t succ_latency, + int fail_count, int64_t fail_latency) { + int64_t now = butil::gettimeofday_us(); + + // Add successful samples + for (int i = 0; i < succ_count; ++i) { + limiter.AddSample(0, succ_latency, now); + } + // Add failed samples + for (int i = 0; i < fail_count; ++i) { + limiter.AddSample(1, fail_latency, now); + } + + // Wait for window to expire and trigger update + bthread_usleep(brpc::policy::FLAGS_auto_cl_sample_window_size_ms * 1000 + 1000); + + // Add one more sample to trigger window submission + limiter.AddSample(0, succ_latency, butil::gettimeofday_us()); Review Comment: `AddSamplesAndTriggerWindow` always adds an extra successful sample (the final `AddSample(0, ...)` used to trigger submission). This changes the requested `succ_count`/`fail_count` totals and skews the effective error rate used by the assertions and the formulas in comments (e.g., the “90% error rate” case becomes 90/(90+10+1)). Consider restructuring the helper so the trigger call is included in `succ_count`/`fail_count` (e.g., add `succ_count - 1` successes before advancing the window, then use the final success as the trigger) so the test inputs match the intended rates. ```suggestion // We want the total number of successes and failures, including the // final trigger sample, to match succ_count and fail_count. // Reserve one sample (success or failure) as the trigger if possible. int pre_succ_count = succ_count; int pre_fail_count = fail_count; bool trigger_is_success = true; if (succ_count > 0) { // Use a success as the trigger: send one fewer success before the window. pre_succ_count = succ_count - 1; trigger_is_success = true; } else if (fail_count > 0) { // No successes requested; use a failure as the trigger instead. pre_fail_count = fail_count - 1; trigger_is_success = false; } else { // Edge case: no samples requested; fall back to previous behavior // of sending a single success solely to trigger the window. pre_succ_count = 0; pre_fail_count = 0; trigger_is_success = true; } // Add successful samples (excluding any reserved trigger sample) for (int i = 0; i < pre_succ_count; ++i) { limiter.AddSample(0, succ_latency, now); } // Add failed samples (excluding any reserved trigger sample) for (int i = 0; i < pre_fail_count; ++i) { limiter.AddSample(1, fail_latency, now); } // Wait for window to expire and trigger update bthread_usleep(brpc::policy::FLAGS_auto_cl_sample_window_size_ms * 1000 + 1000); // Add the final sample to trigger window submission. // If any samples were requested, this trigger is counted in // succ_count/fail_count; otherwise, we add a single success as before. if (succ_count + fail_count > 0) { if (trigger_is_success) { limiter.AddSample(0, succ_latency, butil::gettimeofday_us()); } else { limiter.AddSample(1, fail_latency, butil::gettimeofday_us()); } } else { limiter.AddSample(0, succ_latency, butil::gettimeofday_us()); } ``` -- 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]
