Copilot commented on code in PR #3526:
URL: https://github.com/apache/brpc/pull/3526#discussion_r3950802274
##########
src/brpc/load_balancer.cpp:
##########
@@ -30,7 +33,60 @@ 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. Must be positive: 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 ValidateWarmupCurve(const char*, double v) {
+ return v > 0.0;
+}
+static bool ValidateWarmupMinWeight(const char*, double v) {
+ return v > 0.0 && v <= 1.0;
+}
+BRPC_VALIDATE_GFLAG(lb_warmup_curve, ValidateWarmupCurve);
+BRPC_VALIDATE_GFLAG(lb_warmup_min_weight, ValidateWarmupMinWeight);
+
+
+double WarmupMultiplierImpl(int64_t join_time_us, int64_t now_us) {
+ const int64_t warmup_us = FLAGS_lb_warmup_ms * 1000L;
+ if (warmup_us <= 0 || join_time_us <= 0) {
+ return 1.0;
+ }
Review Comment:
`FLAGS_lb_warmup_ms * 1000L` can overflow `int64_t` for very large warm-up
windows, potentially turning `warmup_us` negative and disabling warm-up
unexpectedly. Consider guarding against overflow (e.g., clamp at `INT64_MAX`,
or validate `lb_warmup_ms <= INT64_MAX/1000`) before multiplying.
##########
src/brpc/policy/weighted_round_robin_load_balancer.cpp:
##########
@@ -182,8 +183,15 @@ int WeightedRoundRobinLoadBalancer::SelectServer(const
SelectIn& in, SelectOut*
size_t remain_servers = s->server_list.size();
while (remain_servers > 0) {
SocketId server_id = GetServerInNextStride(s->server_list, filter,
tls_temp);
+ bool warmup_pass = true;
+ if (remain_servers > 1 && FLAGS_lb_warmup_ms > 0) {
+ warmup_pass = WarmupAccept(
+ s->server_list[s->server_map.at(server_id)].join_time_us,
+ in.begin_time_us);
+ }
Review Comment:
This adds a `server_map.at(server_id)` lookup (and bounds-checking
semantics) in the hot selection loop. Since `GetServerInNextStride(...)`
already has to locate a specific element in `server_list`, consider returning
the selected index (or a pointer/reference to the `Server`) so you can read
`join_time_us` without an extra map lookup and without `at()`-style failure
behavior.
##########
src/brpc/policy/consistent_hashing_load_balancer.h:
##########
@@ -47,6 +47,10 @@ class ConsistentHashingLoadBalancer : public LoadBalancer {
uint32_t hash;
ServerId server_sock;
butil::EndPoint server_addr; // To make sorting stable among all
clients
+ // Time when the server was added, for the warm-up ramp. Not part
+ // of ordering/equality so that re-adding an existing server keeps
+ // its original stamp.
Review Comment:
The comment is ambiguous and may read as contradicting the stated behavior
that remove+add restarts the ramp. Consider clarifying what “re-adding an
existing server” means here (e.g., duplicate `AddServer` without prior removal,
or ring rebuild without membership change) and in which cases `join_time_us` is
expected to be preserved vs. reset.
--
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]
