Copilot commented on code in PR #3526:
URL: https://github.com/apache/brpc/pull/3526#discussion_r3943582797
##########
src/brpc/load_balancer.cpp:
##########
@@ -30,7 +33,57 @@ DEFINE_int32(default_weight_of_wlb, 0, "Default weight value
of Weighted LoadBal
"problems when user is using wlb but forgot to set the weights of
some of their "
"downstream instances. Then these instances will be set
default_weight_of_wlb as "
"their weights. wlb policy degradation is not enabled by
default.");
+DEFINE_int64(lb_warmup_ms, 0,
+ "When positive, a server newly added to a LoadBalancer gets "
+ "lb_warmup_min_weight of its normal traffic share at first and "
+ "ramps up to 100% over this period(ms). 0 disables the warm-up");
+DEFINE_double(lb_warmup_curve, 1.0,
+ "Shape of the warm-up ramp: the weight multiplier is "
+ "max(lb_warmup_min_weight, progress^lb_warmup_curve) where
progress rises "
+ "linearly from 0 to 1 over lb_warmup_ms. 1 ramps linearly, "
+ "larger values keep a new server colder for longer");
BRPC_VALIDATE_GFLAG(show_lb_in_vars, PassValidate);
+BRPC_VALIDATE_GFLAG(lb_warmup_ms, PassValidate);
+DEFINE_double(lb_warmup_min_weight, 0.1,
+ "Floor of the warm-up multiplier, in (0, 1]: the share of "
+ "normal traffic a server gets right after joining, so that "
+ "it still receives a trickle and latency-based policies keep "
+ "observing it");
+static bool ValidateWarmupMinWeight(const char*, double v) {
+ return v > 0.0 && v <= 1.0;
+}
+BRPC_VALIDATE_GFLAG(lb_warmup_curve, PassValidate);
+BRPC_VALIDATE_GFLAG(lb_warmup_min_weight, ValidateWarmupMinWeight);
Review Comment:
-lb_warmup_curve is described as shaping the ramp via
progress^lb_warmup_curve, but the flag currently uses PassValidate and the
implementation silently ignores non-positive values (curve <= 0 behaves like a
linear ramp). Adding validation that curve > 0 makes the runtime behavior match
the documented formula and avoids surprising configurations.
##########
test/brpc_lb_warmup_unittest.cpp:
##########
@@ -0,0 +1,296 @@
+// 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 <map>
+#include <sstream>
+#include <vector>
+#include <gflags/gflags.h>
+#include <gtest/gtest.h>
+#include <unistd.h>
+#include "butil/fast_rand.h"
+#include "butil/time.h"
+#include "brpc/socket.h"
+#include "brpc/load_balancer.h"
+#include "brpc/policy/round_robin_load_balancer.h"
+#include "brpc/policy/weighted_round_robin_load_balancer.h"
+#include "brpc/policy/consistent_hashing_load_balancer.h"
+#include "brpc/policy/locality_aware_load_balancer.h"
+#include "brpc/policy/p2c_ewma_load_balancer.h"
+
+namespace brpc {
+DECLARE_double(lb_warmup_curve);
+DECLARE_double(lb_warmup_min_weight);
+}
+
+namespace {
+
+class SaveRecycle : public brpc::SocketUser {
+ void BeforeRecycle(brpc::Socket* s) { delete this; }
+};
+
+brpc::ServerId CreateServer(const char* addr, const char* tag = "") {
+ butil::EndPoint point;
+ EXPECT_EQ(0, str2endpoint(addr, &point));
+ brpc::ServerId id(8888);
+ brpc::SocketOptions options;
+ options.remote_side = point;
+ options.user = new SaveRecycle;
+ EXPECT_EQ(0, brpc::Socket::Create(options, &id.id));
+ id.tag = tag;
+ return id;
+}
+
+// Select `count' times at `now_us' and return times each server was chosen.
+// Feeds back immediately when the LB asks for it(la).
+std::map<brpc::SocketId, int> CountShares(
+ brpc::LoadBalancer* lb, int count, int64_t now_us,
+ bool changable_weights = false, bool with_request_code = false) {
+ std::map<brpc::SocketId, int> shares;
+ for (int i = 0; i < count; ++i) {
+ brpc::LoadBalancer::SelectIn in = {
+ now_us, changable_weights, with_request_code,
+ with_request_code ? butil::fast_rand() % UINT_MAX : 0u, nullptr };
+ brpc::SocketUniquePtr ptr;
+ brpc::LoadBalancer::SelectOut out(&ptr);
+ if (lb->SelectServer(in, &out) != 0) {
+ continue;
+ }
+ ++shares[ptr->id()];
+ if (out.need_feedback) {
+ brpc::LoadBalancer::CallInfo info;
+ info.begin_time_us = now_us;
+ info.server_id = ptr->id();
+ info.error_code = 0;
+ info.controller = nullptr;
+ lb->Feedback(info);
+ }
+ }
+ return shares;
+}
+
+class LbWarmupTest : public ::testing::Test {
+protected:
+ void SetUp() override {
+ _saved_warmup_ms = brpc::FLAGS_lb_warmup_ms;
+ _saved_curve = brpc::FLAGS_lb_warmup_curve;
+ _saved_min_weight = brpc::FLAGS_lb_warmup_min_weight;
+ }
+ void TearDown() override {
+ brpc::FLAGS_lb_warmup_ms = _saved_warmup_ms;
+ brpc::FLAGS_lb_warmup_curve = _saved_curve;
+ brpc::FLAGS_lb_warmup_min_weight = _saved_min_weight;
+ }
+
+ int64_t _saved_warmup_ms;
+ double _saved_curve;
+ double _saved_min_weight;
+};
Review Comment:
This test fixture manually saves/restores gflags in SetUp/TearDown. In this
repo, tests typically use scoped RAII (e.g. GFLAGS_NAMESPACE::FlagSaver) to
ensure flags are restored even if the test returns early or adds more flag
mutations later; using the standard helper also avoids needing to track
individual flags.
--
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]
