Copilot commented on code in PR #3229: URL: https://github.com/apache/brpc/pull/3229#discussion_r2852984509
########## src/brpc/backup_request_policy.cpp: ########## @@ -0,0 +1,197 @@ +// 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/backup_request_policy.h" + +#include <new> // std::nothrow +#include <gflags/gflags.h> +#include "butil/logging.h" +#include "brpc/reloadable_flags.h" +#include "bvar/reducer.h" +#include "bvar/window.h" +#include "butil/atomicops.h" +#include "butil/time.h" + +namespace brpc { + +DEFINE_double(backup_request_max_ratio, -1, + "Maximum ratio of backup requests to total requests. " + "Value in (0, 1] enables rate limiting. Values <= 0 disable it " + "(-1 is default). Can be overridden per-channel via " + "ChannelOptions.backup_request_max_ratio. " + "Note: takes effect at Channel::Init() time; changing this flag " + "at runtime does not affect already-created channels."); + +static bool validate_backup_request_max_ratio(const char*, double v) { + if (v <= 0) return true; // non-positive means disabled + if (v <= 1.0) return true; + LOG(ERROR) << "Invalid backup_request_max_ratio=" << v + << ", must be <= 0 (disabled) or in (0, 1]"; + return false; +} +BRPC_VALIDATE_GFLAG(backup_request_max_ratio, + validate_backup_request_max_ratio); + +DEFINE_int32(backup_request_ratio_window_size_s, 10, + "Window size in seconds for computing the backup request ratio. " + "Must be >= 1."); + +static bool validate_backup_request_ratio_window_size_s( + const char*, int32_t v) { + if (v >= 1) return true; + LOG(ERROR) << "Invalid backup_request_ratio_window_size_s=" << v + << ", must be >= 1"; + return false; +} +BRPC_VALIDATE_GFLAG(backup_request_ratio_window_size_s, + validate_backup_request_ratio_window_size_s); + +DEFINE_int32(backup_request_ratio_update_interval_s, 5, + "Interval in seconds between ratio cache updates. Must be >= 1."); + +static bool validate_backup_request_ratio_update_interval_s( + const char*, int32_t v) { + if (v >= 1) return true; + LOG(ERROR) << "Invalid backup_request_ratio_update_interval_s=" << v + << ", must be >= 1"; + return false; +} +BRPC_VALIDATE_GFLAG(backup_request_ratio_update_interval_s, + validate_backup_request_ratio_update_interval_s); + +// Standalone statistics module for tracking backup/total request ratio +// within a sliding time window. +class BackupRateLimiter { +public: + BackupRateLimiter(double max_backup_ratio, + int window_size_seconds, + int update_interval_seconds) + : _max_backup_ratio(max_backup_ratio) + , _update_interval_us(update_interval_seconds * 1000000LL) + , _total_window(&_total_count, window_size_seconds) + , _backup_window(&_backup_count, window_size_seconds) + , _cached_ratio(0.0) + , _last_update_us(0) { + } + + // All atomic operations use relaxed ordering intentionally. + // This is best-effort rate limiting: a slightly stale ratio is + // acceptable for approximate throttling. + bool ShouldAllow() const { + const int64_t now_us = butil::cpuwide_time_us(); + int64_t last_us = _last_update_us.load(butil::memory_order_relaxed); + double ratio = _cached_ratio.load(butil::memory_order_relaxed); + + if (now_us - last_us >= _update_interval_us) { + if (_last_update_us.compare_exchange_strong( + last_us, now_us, butil::memory_order_relaxed)) { + int64_t total = _total_window.get_value(); + int64_t backup = _backup_window.get_value(); + ratio = (total > 0) ? static_cast<double>(backup) / total : 0.0; + _cached_ratio.store(ratio, butil::memory_order_relaxed); + } + } + + // max_backup_ratio >= 1.0 means no limit (ratio cannot exceed 1.0). + return _max_backup_ratio >= 1.0 || ratio < _max_backup_ratio; + } + + void OnRPCEnd(const Controller* controller) { + _total_count << 1; + if (controller->has_backup_request()) { + _backup_count << 1; + } + } + +private: + double _max_backup_ratio; + int64_t _update_interval_us; + + bvar::Adder<int64_t> _total_count; + bvar::Adder<int64_t> _backup_count; + bvar::Window<bvar::Adder<int64_t>> _total_window; + bvar::Window<bvar::Adder<int64_t>> _backup_window; + + mutable butil::atomic<double> _cached_ratio; + mutable butil::atomic<int64_t> _last_update_us; +}; + +// Internal BackupRequestPolicy that composes a BackupRateLimiter +// for ratio-based suppression. +class RateLimitedBackupPolicy : public BackupRequestPolicy { +public: + RateLimitedBackupPolicy(int32_t backup_request_ms, + double max_backup_ratio, + int window_size_seconds, + int update_interval_seconds) + : _backup_request_ms(backup_request_ms) + , _rate_limiter(max_backup_ratio, window_size_seconds, + update_interval_seconds) { + } + + int32_t GetBackupRequestMs(const Controller* /*controller*/) const override { + return _backup_request_ms; + } + + bool DoBackup(const Controller* /*controller*/) const override { + return _rate_limiter.ShouldAllow(); + } + + void OnRPCEnd(const Controller* controller) override { + _rate_limiter.OnRPCEnd(controller); + } + +private: + int32_t _backup_request_ms; + BackupRateLimiter _rate_limiter; +}; + +BackupRequestPolicy* CreateRateLimitedBackupPolicy( + int32_t backup_request_ms, + double max_backup_ratio, + int window_size_seconds, + int update_interval_seconds) { + if (backup_request_ms < 0) { + LOG(ERROR) << "Invalid backup_request_ms=" << backup_request_ms + << ", must be >= 0"; + return NULL; + } + if (max_backup_ratio <= 0 || max_backup_ratio > 1.0) { + LOG(ERROR) << "Invalid max_backup_ratio=" << max_backup_ratio + << ", must be in (0, 1]"; + return NULL; + } + if (window_size_seconds < 1) { + LOG(ERROR) << "Invalid window_size_seconds=" << window_size_seconds + << ", must be >= 1"; + return NULL; + } + if (update_interval_seconds < 1) { + LOG(ERROR) << "Invalid update_interval_seconds=" + << update_interval_seconds << ", must be >= 1"; + return NULL; + } Review Comment: `CreateRateLimitedBackupPolicy()` checks `window_size_seconds < 1` but does not check an upper bound. Since `bvar::Window` enforces a max window size (via `ReducerSampler::MAX_SECONDS_LIMIT`, currently 3600s) and will `CHECK` on invalid sizes, the factory should validate `window_size_seconds <= 3600` (or whatever limit bvar enforces) and return NULL with `LOG(ERROR)` rather than allowing a crash. ########## src/brpc/backup_request_policy.cpp: ########## @@ -0,0 +1,197 @@ +// 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/backup_request_policy.h" + +#include <new> // std::nothrow +#include <gflags/gflags.h> +#include "butil/logging.h" +#include "brpc/reloadable_flags.h" +#include "bvar/reducer.h" +#include "bvar/window.h" +#include "butil/atomicops.h" +#include "butil/time.h" + +namespace brpc { + +DEFINE_double(backup_request_max_ratio, -1, + "Maximum ratio of backup requests to total requests. " + "Value in (0, 1] enables rate limiting. Values <= 0 disable it " + "(-1 is default). Can be overridden per-channel via " + "ChannelOptions.backup_request_max_ratio. " + "Note: takes effect at Channel::Init() time; changing this flag " + "at runtime does not affect already-created channels."); + +static bool validate_backup_request_max_ratio(const char*, double v) { + if (v <= 0) return true; // non-positive means disabled + if (v <= 1.0) return true; + LOG(ERROR) << "Invalid backup_request_max_ratio=" << v + << ", must be <= 0 (disabled) or in (0, 1]"; + return false; Review Comment: The gflag validator for `backup_request_max_ratio` rejects values `> 1.0`, but `Channel::InitChannelOptions()` has logic to clamp `max_ratio > 1.0` to `1.0` with `LOG(WARNING)`. As-is, the clamping path can never apply to the global flag (because the validator prevents setting it), which is inconsistent and makes behavior depend on whether the value comes from the gflag or per-channel option. Consider either (1) relaxing the validator to allow `> 1.0` and relying on the clamping/warning at init-time, or (2) removing the clamping logic and treating `> 1.0` as an error uniformly. ```suggestion "Value > 0 enables rate limiting. Values <= 0 disable it " "(-1 is default). Can be overridden per-channel via " "ChannelOptions.backup_request_max_ratio. " "Note: takes effect at Channel::Init() time; changing this flag " "at runtime does not affect already-created channels. " "Values > 1.0 will be clamped to 1.0 at Channel::Init()."); static bool validate_backup_request_max_ratio(const char*, double v) { // Non-positive means disabled; positive values are allowed and // will be interpreted (and clamped if needed) at Channel::Init(). return true; ``` ########## src/brpc/backup_request_policy.cpp: ########## @@ -0,0 +1,197 @@ +// 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/backup_request_policy.h" + +#include <new> // std::nothrow +#include <gflags/gflags.h> +#include "butil/logging.h" +#include "brpc/reloadable_flags.h" +#include "bvar/reducer.h" +#include "bvar/window.h" +#include "butil/atomicops.h" +#include "butil/time.h" + +namespace brpc { + +DEFINE_double(backup_request_max_ratio, -1, + "Maximum ratio of backup requests to total requests. " + "Value in (0, 1] enables rate limiting. Values <= 0 disable it " + "(-1 is default). Can be overridden per-channel via " + "ChannelOptions.backup_request_max_ratio. " + "Note: takes effect at Channel::Init() time; changing this flag " + "at runtime does not affect already-created channels."); + +static bool validate_backup_request_max_ratio(const char*, double v) { + if (v <= 0) return true; // non-positive means disabled + if (v <= 1.0) return true; + LOG(ERROR) << "Invalid backup_request_max_ratio=" << v + << ", must be <= 0 (disabled) or in (0, 1]"; + return false; +} +BRPC_VALIDATE_GFLAG(backup_request_max_ratio, + validate_backup_request_max_ratio); + +DEFINE_int32(backup_request_ratio_window_size_s, 10, + "Window size in seconds for computing the backup request ratio. " + "Must be >= 1."); + +static bool validate_backup_request_ratio_window_size_s( + const char*, int32_t v) { + if (v >= 1) return true; + LOG(ERROR) << "Invalid backup_request_ratio_window_size_s=" << v + << ", must be >= 1"; + return false; +} +BRPC_VALIDATE_GFLAG(backup_request_ratio_window_size_s, + validate_backup_request_ratio_window_size_s); + Review Comment: `backup_request_ratio_window_size_s` (and `CreateRateLimitedBackupPolicy`'s `window_size_seconds`) are only validated for `>= 1`, but `bvar::Window` ultimately calls `ReducerSampler::set_window_size()` which rejects values `> 3600` and then triggers a `CHECK_EQ(0, ...)` in the `Window` constructor. This means a user can crash the process by setting the flag / passing a large window. Please add an upper-bound validation (<= 3600) both for the gflag validator and the factory parameter checks, and log a clear error instead of crashing. ########## src/brpc/channel.cpp: ########## @@ -242,6 +271,37 @@ int Channel::InitChannelOptions(const ChannelOptions* options) { if (!cg.empty() && (::isspace(cg.front()) || ::isspace(cg.back()))) { butil::TrimWhitespace(cg, butil::TRIM_ALL, &cg); } + + // Create rate-limited backup policy if configured. + // User-provided backup_request_policy takes precedence. + if (_options.backup_request_policy != NULL && + _options.backup_request_max_ratio > 0) { + LOG(WARNING) << "backup_request_max_ratio is ignored because " + "backup_request_policy is already set"; + } + // Per-channel option takes precedence over the global gflag. + double max_ratio = _options.backup_request_max_ratio; + if (max_ratio < 0) { + max_ratio = FLAGS_backup_request_max_ratio; + } Review Comment: The warning about `backup_request_max_ratio` being ignored only triggers when the *per-channel* field is `> 0`. If the user sets `backup_request_policy` and leaves `backup_request_max_ratio = -1` (default) while the global `FLAGS_backup_request_max_ratio` is positive, the ratio limit is still effectively configured but no warning is emitted. Consider computing the effective `max_ratio` first (including gflag fallback/clamp) and warning when `backup_request_policy != NULL && max_ratio > 0` to match the stated precedence rules and avoid silent confusion. -- 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]
