xds: add weighted round robin LB policy support #9873

YifeiZhuang · 2023-02-03T00:53:43Z

implementation design doc: go/grpc-java-wrr

1. to merge orca api remove listener 2. to merge metric report api rqs 3. to settle the package

RoundRobinLB picker issue, e.g. size = 5, index = 4 pick1: i = index = 5 pick2: i = index = 6 pick1 : oldi = 5, i = 0, index = 0 pick2: oldi = 6, i = 1, index not updated = 0. pick1 return subchanel[0], pick2 return subchannel[1] next time, it still return subchannel[1], it gets picked more often;

xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancer.java

larry-safran · 2023-02-11T01:19:58Z

xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancer.java

+        double newWeight = subchannel.getWeight();
+        scheduler.add(i, newWeight > 0 ? newWeight : avgWeight);
+      }
+      schedulerRef.set(scheduler);


Because you are completely replacing the scheduler each time you update weights, if something has a large weight and something else has a minimum weight the second one might never get used.
One way of dealing with this would be for ObjectState to have a flag indicating whether this was added from a pick, then for all of the ones that weren't added from a pick you could use the old deadline (or the smaller of the old deadline and the newly calculated one). You could have the flag passed to scheduler.add() and all of the work done in the add method.

Absolutely true.
Note stubby has similar "completion ratio" mechanism, that gives credits to subchannels in the previous state when updating to the next state with the new weight. This way, the minimum weight channel can possibly be picked.
The current implementation is very simplified. I'll make it as a future improvement and I will capture it in the design doc.

Because you are completely replacing the scheduler each time you update weights, if something has a large weight and something else has a minimum weight the second one might never get used.

Randomizing the scheduler each creation should prevent that. Worst-case, if a schedule is used for only a single pick after being created, then that is the same as a WRR implementation that has weighted ranges for each choice and uses a random number to choose (same approach as weighted_target).

The code right now does not seem to randomize the initial scheduler state, but it will need to before de-experimentalizing.

xds/src/test/java/io/grpc/xds/WeightedRoundRobinLoadBalancerTest.java

ejona86 · 2023-02-14T00:05:40Z

Can you rebase this on master because #9875 is merged? I would have looked at just the commits after that other PR, but there were so many; in the future you could squash commits that were just used during development before creating the PR.

xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancer.java

ejona86

Still haven't found enough time to get through it all :-/. Sending what I have.

xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancerProvider.java

xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancer.java

xds/src/main/java/io/grpc/xds/LoadBalancerConfigFactory.java

xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancerProvider.java

ejona86

Happened to go a bit further, but this is still incomplete. All of these comments are minor.

ejona86 · 2023-02-23T00:44:29Z

xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancer.java

+      }
+
+      WeightedRoundRobinLoadBalancerConfig build() {
+        return new WeightedRoundRobinLoadBalancerConfig(blackoutPeriodNanos,


FYI: passing this to the Config constructor is convenient, and Config will have access to the fields even if they are private. (no need to change though)

xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancer.java

ejona86 · 2023-02-23T21:00:28Z

xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancer.java

+    private volatile long lastUpdated;
+    private volatile long nonEmptySince;
+    private volatile double weight;
+    private volatile WeightedRoundRobinLoadBalancerConfig config;


Why is this volatile? It appears to only be accessed in the synchronization context.

oh it appears so.
It can be used by pick() from SubchannelStateListener from transport thread, or here in the LB thread in weightUpdateTask or acceptResolvedAddresses, all in sync context.

xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancer.java

core/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java

xds/src/main/java/io/grpc/xds/LoadBalancerConfigFactory.java

ejona86

There's two non-FYI remaining comments from the previous part of the review that I'd like to see done. They are related; passing config.enableOobLoadReport to the picker's constructor would allow making config non-volatile.

After those two previous comments and the comments requiring change in this review, things look good. I didn't look at tests, but Larry did previously.

xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancer.java

core/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java

xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancer.java

xds/src/main/java/io/grpc/xds/LoadBalancerConfigFactory.java

xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancer.java

YifeiZhuang added 18 commits January 23, 2023 16:27

refactor round robin LB

1f7bbd8

rename abstract*

73cf208

refactor round robin LB

12e7928

rename abstract*

2727d9b

temp: add weightedroundrobinimpl

764c9a9

1. to merge orca api remove listener 2. to merge metric report api rqs 3. to settle the package

add weighted round robin picker and scheduler

afa10fb

comments, and add afterSubchannelUpdate

d4f785d

format

60af73c

move abstraction to composition

4228f97

remove listener

cf3d640

use original round robin wl

975eeed

add subchannel listener

5300199

add test

2458ac9

Merge branch 'master' of https://github.com/grpc/grpc-java into wrr-impl

9cd33b6

add more tests

23231eb

add provider test

3d51405

fix current picker

e6886c1

YifeiZhuang changed the title ~~implement weighted round robin LB policy~~ xds: add weighted round robin LB policy support Feb 9, 2023

YifeiZhuang requested review from ejona86 and larry-safran February 10, 2023 00:16

YifeiZhuang marked this pull request as ready for review February 10, 2023 00:19

remove virtual time, change comment

3da127e

larry-safran reviewed Feb 11, 2023

View reviewed changes

YifeiZhuang added 2 commits February 13, 2023 11:56

fix avg weight

cb31730

Merge branch 'master' of https://github.com/grpc/grpc-java into wrr-impl

01a7d1a

YifeiZhuang requested a review from larry-safran February 13, 2023 20:19

larry-safran approved these changes Feb 13, 2023

View reviewed changes

Merge branch 'master' of https://github.com/grpc/grpc-java into wrr-impl

6e496da

yousukseung reviewed Feb 15, 2023

View reviewed changes

xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancer.java Outdated Show resolved Hide resolved

YifeiZhuang added 3 commits February 16, 2023 15:28

add env variable

6dfa072

Merge branch 'master' of https://github.com/grpc/grpc-java into wrr-impl

f786098

parse wrr proto

0bdce32

ejona86 reviewed Feb 23, 2023

View reviewed changes

add test scheduler

3979ef8

ejona86 reviewed Feb 23, 2023

View reviewed changes

YifeiZhuang added 3 commits February 23, 2023 18:22

fix comments, timer, volatile, etc

0704a69

bazel checksum, and use ticker

fc0d3dd

infTime = nanoTime() + MAX_VALUE

f728281

YifeiZhuang requested a review from ejona86 February 24, 2023 21:59

ejona86 approved these changes Feb 25, 2023

View reviewed changes

minor fix

1ef6221

YifeiZhuang merged commit 8d12baa into grpc:master Feb 27, 2023

YifeiZhuang deleted the wrr-impl branch February 27, 2023 18:39

YifeiZhuang mentioned this pull request Mar 2, 2023

xds, wrr: randomize the initial deadline in the scheduler #9922

Merged

github-actions bot locked as resolved and limited conversation to collaborators May 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xds: add weighted round robin LB policy support #9873

xds: add weighted round robin LB policy support #9873

YifeiZhuang commented Feb 3, 2023

larry-safran Feb 11, 2023

YifeiZhuang Feb 13, 2023

ejona86 Feb 14, 2023

ejona86 commented Feb 14, 2023

ejona86 left a comment

ejona86 left a comment

ejona86 Feb 23, 2023

ejona86 Feb 23, 2023

YifeiZhuang Feb 24, 2023 •

edited

Loading

ejona86 left a comment •

edited

Loading

xds: add weighted round robin LB policy support #9873

xds: add weighted round robin LB policy support #9873

Conversation

YifeiZhuang commented Feb 3, 2023

larry-safran Feb 11, 2023

Choose a reason for hiding this comment

YifeiZhuang Feb 13, 2023

Choose a reason for hiding this comment

ejona86 Feb 14, 2023

Choose a reason for hiding this comment

ejona86 commented Feb 14, 2023

ejona86 left a comment

Choose a reason for hiding this comment

ejona86 left a comment

Choose a reason for hiding this comment

ejona86 Feb 23, 2023

Choose a reason for hiding this comment

ejona86 Feb 23, 2023

Choose a reason for hiding this comment

YifeiZhuang Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

ejona86 left a comment • edited Loading

Choose a reason for hiding this comment

YifeiZhuang Feb 24, 2023 •

edited

Loading

ejona86 left a comment •

edited

Loading