Copilot commented on code in PR #3229: URL: https://github.com/apache/brpc/pull/3229#discussion_r2852740847
########## src/brpc/backup_request_policy.cpp: ########## @@ -0,0 +1,185 @@ +// 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 <gflags/gflags.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]. -1 means no limit (default). Can be overridden " + "per-channel via ChannelOptions.backup_request_max_ratio. " Review Comment: Documentation inconsistency: The help text states "Value in (0, 1]" which excludes 0, but the validator accepts `v <= 0` and treats it as "disabled". This creates confusion about whether 0 is a valid value. The help text should clarify that values <= 0 disable rate limiting, e.g., "Value in (0, 1] enables rate limiting. Values <= 0 disable it (-1 is default)." ```suggestion "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. " ``` ########## src/brpc/channel.h: ########## @@ -116,11 +116,24 @@ struct ChannelOptions { // Default: NULL const Authenticator* auth; + // Maximum ratio of backup requests to total requests within a sliding + // time window. When the ratio reaches or exceeds this value, backup + // requests are suppressed. Value in (0, 1]. -1 means no limit. + // Only effective when backup_request_ms >= 0 and backup_request_policy + // is NULL (i.e. no custom policy). When effective, an internal + // rate-limited BackupRequestPolicy is created and used automatically. + // Default: -1 (no limit, same as FLAGS_backup_request_max_ratio) Review Comment: Documentation inconsistency: The comment states "Value in (0, 1]" which excludes 0, but the implementation in channel.cpp:278 checks `max_ratio > 0`, meaning 0 is treated as "disabled" (no policy created). The gflag validator also accepts values <= 0 as "disabled". The documentation should be updated to clarify that values <= 0 disable rate limiting, with -1 using the global gflag default and 0 explicitly disabling it, e.g., "Value in (0, 1] enables rate limiting. -1 (default) uses global gflag. 0 or negative values explicitly disable rate limiting." ```suggestion // requests are suppressed. Value in (0, 1] enables rate limiting. // Values <= 0 disable rate limiting (no internal policy is created). // -1 (default) means use the global gflag value. // Only effective when backup_request_ms >= 0 and backup_request_policy // is NULL (i.e. no custom policy). When effective, an internal // rate-limited BackupRequestPolicy is created and used automatically. // Default: -1 (use global FLAGS_backup_request_max_ratio) ``` ########## src/brpc/channel.cpp: ########## @@ -242,6 +257,35 @@ 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; + } + if (max_ratio > 1.0) { + LOG(WARNING) << "backup_request_max_ratio=" << max_ratio + << " is out of range (0, 1], clamped to 1.0"; + max_ratio = 1.0; + } + if (max_ratio > 0 && _options.backup_request_policy == NULL && + _options.backup_request_ms >= 0) { + BackupRequestPolicy* policy = CreateRateLimitedBackupPolicy( + _options.backup_request_ms, max_ratio, + FLAGS_backup_request_ratio_window_size_s, + FLAGS_backup_request_ratio_update_interval_s); + if (policy) { + _options.backup_request_policy = policy; + _owns_backup_policy = true; Review Comment: Missing NULL check: `CreateRateLimitedBackupPolicy` can return NULL on invalid parameters, but the return value is not checked before assignment and setting `_owns_backup_policy = true`. While the parameters should be valid due to prior validation (max_ratio is in (0,1] and gflags are validated >= 1), adding a defensive NULL check would make the code more robust. If NULL is returned, it should log an error and not set `_owns_backup_policy = true`. ```suggestion _owns_backup_policy = true; } else { LOG(ERROR) << "Fail to create rate-limited backup policy with " << "backup_request_ms=" << _options.backup_request_ms << ", max_ratio=" << max_ratio << ", ratio_window_size_s=" << FLAGS_backup_request_ratio_window_size_s << ", ratio_update_interval_s=" << FLAGS_backup_request_ratio_update_interval_s; ``` -- 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]
