Skip to content

fix WeightedRandomizedLoadBalancer skipping the last server - #3426

Open
Nas01010101 wants to merge 1 commit into
apache:masterfrom
Nas01010101:fix/wr-lb-off-by-one
Open

fix WeightedRandomizedLoadBalancer skipping the last server#3426
Nas01010101 wants to merge 1 commit into
apache:masterfrom
Nas01010101:fix/wr-lb-off-by-one

Conversation

@Nas01010101

Copy link
Copy Markdown

What problem does this PR solve?

WeightedRandomizedLoadBalancer::SelectServer() never selects the server that was added last, and over-selects the first one.

Add() fills Server::current_weight_sum with an inclusive prefix sum. SelectServer() draws random_weight from butil::fast_rand_less_than(weight_sum), i.e. from [0, weight_sum - 1], and then lower_bound()s that value against those prefix sums. lower_bound() returns the first server whose prefix sum is >= random_weight, but a server owns the half-open range [prefix(i-1), prefix(i)), so the predicate has to be > random_weight.

Two consequences: the first server also serves random_weight == prefix(0), so it gets one slot too many; and the last server is never selected at all, because random_weight can never reach weight_sum.

With four servers of equal weight the measured distribution is 49.8 / 25.1 / 25.1 / 0.0 percent instead of 25 percent each. The last server only receives traffic through the fallback loop, when every server picked at random happens to be unavailable.

What is changed and the side effects?

Changed: search for random_weight + 1, so lower_bound() lands on the first prefix sum strictly greater than random_weight. random_weight + 1 is at most weight_sum, which is exactly the last prefix sum, so the iterator is still always valid.

Side effects:

  • Performance effects: none, the comparison is unchanged.
  • Breaking backward compatibility: no. Traffic shifts towards the configured weights.

The existing weighted_randomized test does not catch this. Its servers have weights 3/2/5/10 and it only asserts that every rate is within 0.5x~2x of the expected one. The weight-10 server measures 0.453 before this change and 0.5055 after it, and both are inside that band. The off-by-one predicts that number exactly: the server should own 10 of 20 slots but owns 9, i.e. 0.45.

This PR adds weighted_randomized_equal_weight, which uses equal weights so a single misplaced slot is visible, and checks the rates within 0.9x1.1x. On master the last server is selected 0 times out of 40000 and the test fails; with this change all four land within 0.24760.25435.

Check List:

  • brpc_load_balancer_unittest passes in full: [ PASSED ] 17 tests.
  • The new test was run with --gtest_repeat=40 without a failure.
  • Verified the new test fails without the one-line change and passes with it.

Tested on macOS/arm64 with clang. I was not able to build on Linux/gcc locally; CI will cover that.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes an off-by-one error in brpc::policy::WeightedRandomizedLoadBalancer::SelectServer() that prevented the last server in the weighted prefix-sum list from ever being selected (and slightly over-selected the first), and adds a targeted unit test to catch this regression.

Changes:

  • Fix selection logic by searching lower_bound(random_weight + 1) against inclusive prefix sums.
  • Add weighted_randomized_equal_weight unit test with equal weights and tighter distribution bounds to reliably detect the bug.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/brpc/policy/weighted_randomized_load_balancer.cpp Adjusts the weighted random selection to correctly map random draws onto prefix-sum ranges.
test/brpc_load_balancer_unittest.cpp Adds a new equal-weight distribution test to expose the prior off-by-one behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +1071 to +1072
// 0.9x ~ 1.1x of the expected rate, which is more than 20 standard
// deviations away from the mean at this number of runs.
SelectServer() draws random_weight from fast_rand_less_than(weight_sum),
i.e. from [0, weight_sum - 1], and then lower_bound()s it against
Server::current_weight_sum, which Add() fills with an inclusive prefix
sum. lower_bound() returns the first server whose prefix sum is >=
random_weight, but a server owns the half-open range
[prefix(i-1), prefix(i)), so the predicate has to be > random_weight.

Because of that the first server in the list also serves
random_weight == prefix(0) and the last server never serves anything at
all, since random_weight can never reach weight_sum. With four servers of
equal weight the measured distribution is 49.8/25.1/25.1/0.0 percent
instead of 25 percent each.

Search for random_weight + 1 so that lower_bound() lands on the first
prefix sum strictly greater than random_weight.

The existing weighted_randomized test does not catch this: its servers
have weights 3/2/5/10 and it only asserts that each rate is within
0.5x~2x of the expected one. The weight-10 server measures 0.448 before
this change and 0.494 after it, both inside that band. Add
weighted_randomized_equal_weight, which uses equal weights so that a
single misplaced slot is visible, and check the rates within 0.9x~1.1x.

Signed-off-by: Anas <156536069+Nas01010101@users.noreply.github.com>
@Nas01010101
Nas01010101 force-pushed the fix/wr-lb-off-by-one branch from e0cf753 to 9637f6b Compare August 3, 2026 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants