Remove redundant SubchannelPicker refreshes in RoundRobinLoadBalancer #4607

njhill · 2018-07-01T01:45:35Z

The "active" list and corresponding circular index used by RoundRobinLoadBalancer's picker is currently refreshed via a call to helper.updateBalancingState() after any change to a Subchannel's state or the resolved address groups, regardless of whether that change actually results in a change to the contents of the list.

In fact it appears that the majority of Picker refreshes may be no-ops - for example any time a new EAG is discovered while there are already active Subchannels, its new Subchannel is added in IDLE state followed by an immediate Picker refresh which will be unnecessary because the list only contains READY Subchannels. A similar pattern occurs during Subchannel removal/shutdown.

These avoidable perturbations to the load distribution may be exacerbating the behaviour described in #4462.

This PR makes changes to ensure this doesn't happen, and includes:

Making it so that a Subchannel exists in the subchannels map if and only if its state is not SHUTDOWN. This also facilitates a simplification to the stickiness picker logic where list containment no longer needs to be checked when (re)registering a stickiness value.
Adding an EmptyPicker class, since the picker parameters and logic are disjoint from the non-empty case

- Ensure active subchannel list and round-robin index is only regenerated/refreshed when it changes - Make it so that Subchannels exist in subchannels map iff their state != SHUTDOWN - Add EmptyPicker class since logic for this case is disjoint from the non-empty case

njhill · 2018-07-12T22:42:39Z

@zhangkun83 any interest in this change?

carl-mastrangelo

@dapengzhang0 can you take a look?

carl-mastrangelo · 2018-07-09T20:57:12Z

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

    private final Map<EquivalentAddressGroup, Subchannel> subchannels =
        new HashMap<EquivalentAddressGroup, Subchannel>();
+    // true when map contains at least one Subchannel in READY state
+    private boolean ready = false;


you can drop the = false as it is default.

@carl-mastrangelo

per @carl-mastrangelo's review comment

njhill · 2018-07-13T23:03:11Z

Thanks @carl-mastrangelo. I noticed your PR #4643 has some bits in common with this - might it make sense for EmptyPicker and FailedPicker to be protected static classes in the LoadBalancer superclass?

njhill · 2018-07-29T21:10:11Z

@carl-mastrangelo @dapengzhang0 sorry to bug, wondering if you might have a chance to look at this?

carl-mastrangelo

Initial round of comments, sorry for the delay. This code is complicated and take a lot of time to read.

carl-mastrangelo · 2018-07-31T00:29:22Z

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

      for (EquivalentAddressGroup addressGroup : removedAddrs) {
        Subchannel subchannel = subchannels.remove(addressGroup);
-        subchannel.shutdown();
+        update = update || isReady(subchannel); // no need to update if channel was already excluded


Isn't this missing for the addedAddrs ? i.e. why only update on removing addresses?

The main intention is to avoid calling updateBalancingState() in cases that won't result in any change to the Picker state, particularly when it already has active endpoints.

Any new addedAddrs are added in IDLE state and so will have no effect on the Picker state if already has at least one READY subchannel (and if it doesn't then ready will be false and so we update anyhow).

carl-mastrangelo · 2018-07-31T00:31:05Z

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

-      if (stateInfo.getState() == SHUTDOWN && stickinessState != null) {
+      Ref<ConnectivityStateInfo> stateInfoRef = getSubchannelStateInfoRef(subchannel);
+      ConnectivityState stateBefore = stateInfoRef.value.getState();
+      if (stateBefore == SHUTDOWN) {


This should be an error, as there should be no more events after the SC has shut down.

The state in the subchannel attribute is now set to SHUTDOWN when the EAG is removed in the handleResolvedAddressGroups callback (at the same time as subchannel.shutdown() is called and that it's removed from this LB's subchannels map - see line 220).

This is because from the load balancer's pov it makes more sense to consider it in a non-ready state as soon as the NameResolver reports it removed, rather than when a subsequent event is received that the shutdown is complete (i.e. here).

So this is really catching the "normal" SHUTDOWN state change following the subchannel's prior removal.

carl-mastrangelo · 2018-07-31T00:32:25Z

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

+      if (newState == SHUTDOWN && stickinessState != null) {
        stickinessState.remove(subchannel);
      }
      if (subchannels.get(subchannel.getAddresses()) != subchannel) {


@zhangkun83 How can this case happen?

The balancer could shutdown a subchannel and create a new one for the same address because of churn on the address list. ManagedChannelImpl may still have state updates for the previous subchannel even after the new one is created. The balancer should ignore those updates.

carl-mastrangelo · 2018-07-31T00:33:59Z

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

      }
-      if (stateInfo.getState() == IDLE) {
+      if (newState == IDLE) {
        subchannel.requestConnection();


@zhangkun83 isn't this also redundant? When a new channel is created the connection is already requested. What will calling it twice do? Also, won't this prevent channels from ever going idle?

RR needs to actively maintain connections to all servers even in absence of RPCs. This line makes sure subchannel reconnects after the initial connections is broken. It's intentional for RR to keep subchannels from going idle. This doesn't stop the top-level channel from entering idle mode after a certain time without RPC.

carl-mastrangelo · 2018-07-31T00:36:54Z

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

-      updateBalancingState(getAggregatedState(), getAggregatedError());
+      stateInfoRef.value = stateInfo;
+      if (!ready || (newState == READY ^ stateBefore == READY)) {
+        updateBalancingState(null, null);


nit: we generally avoid using literals that are not obvious. IN this case, consider naming the literal nulls like:

updateBalancingState(/*connectivityState= */ null, /* status= */ null);

carl-mastrangelo · 2018-07-31T00:38:27Z

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

    private void updateBalancingState(ConnectivityState state, Status error) {
      List<Subchannel> activeList = filterNonFailingSubchannels(getSubchannels());
-      helper.updateBalancingState(state, new Picker(activeList, error, stickinessState));
+      ready = !activeList.isEmpty();


The logic here is starting to get complicated, and it is no longer "obviously" correct. Can you please document the params?

carl-mastrangelo · 2018-07-31T00:40:40Z

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

+        state = ready ? READY : getAggregatedState();
+      }
+      SubchannelPicker picker = ready ? new Picker(activeList, stickinessState) :
+          new EmptyPicker(error != null ? error : getAggregatedError());


there is a lot of conditional inline logic here which is hard to visually follow. For example, why does error take precedence over aggregated error? If the params are meant to be optional, it would be better expressed as an overload.

Agree, I should restructure this to be more explicit/readable.

Some of it is to avoid unnecessary computation - for example getAggregatedState() is only applicable when the filtered activeList is empty, and not in the handleNameResolutionError() callback case. Similar thing for error - it only applies in the empty active list case, where it's determine by getAggregatedError() unless this is triggered by handleNameResolutionError() in which case the name resolver error is relayed.

carl-mastrangelo · 2018-07-31T00:42:49Z

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

+      List<Subchannel> readySubchannels = null;
      for (Subchannel subchannel : subchannels) {
-        if (getSubchannelStateInfoRef(subchannel).value.getState() == READY) {
+        if (isReady(subchannel)) {


@zhangkun83 isn't this going to be an expensive linear time operation? Why is the list scanned each time, rather than Subchannel state change?

What do you mean by "each time"? This method is called only when Subchannel state changes.

zhangkun83

Please merge master to your branch since #4462 is in.

Instead of such an invasive change, have you considered just changing updateBalancingState() so that it saves the current picker, compares the new picker with the current one, and only calls helper.updateBalancingState() if they differ?

zhangkun83 · 2018-08-01T20:30:30Z

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

+      if (newState == SHUTDOWN && stickinessState != null) {
        stickinessState.remove(subchannel);
      }
      if (subchannels.get(subchannel.getAddresses()) != subchannel) {


The balancer could shutdown a subchannel and create a new one for the same address because of churn on the address list. ManagedChannelImpl may still have state updates for the previous subchannel even after the new one is created. The balancer should ignore those updates.

zhangkun83 · 2018-08-01T20:35:13Z

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

      }
-      if (stateInfo.getState() == IDLE) {
+      if (newState == IDLE) {
        subchannel.requestConnection();


RR needs to actively maintain connections to all servers even in absence of RPCs. This line makes sure subchannel reconnects after the initial connections is broken. It's intentional for RR to keep subchannels from going idle. This doesn't stop the top-level channel from entering idle mode after a certain time without RPC.

zhangkun83 · 2018-08-01T20:36:53Z

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

+      List<Subchannel> readySubchannels = null;
      for (Subchannel subchannel : subchannels) {
-        if (getSubchannelStateInfoRef(subchannel).value.getState() == READY) {
+        if (isReady(subchannel)) {


What do you mean by "each time"? This method is called only when Subchannel state changes.

njhill · 2018-08-01T23:51:06Z

Thanks @carl-mastrangelo @zhangkun83 for looking at this.

have you considered just changing updateBalancingState() so that it saves the current picker, compares the new picker with the current one, and only calls helper.updateBalancingState() if they differ?

Yes this was an initial thought, but it seemed kind of wrong/wasteful for me to regenerate and compare the new picker in all these cases where it's easily known at the point of the change that it will have no impact on the picker state, i.e. in the majority of handleResolvedAddressGroups() cases.

I will merge latest from master in, and have a go at making the logic simpler / easier to read. Part of the problem may actually be that I was trying to minimize the invasiveness by tweaking the existing structure. If that still isn't palatable then I agree just doing the picker comparison would be a much smaller change and will still achieve the primary goal.

Conflicts: core/src/main/java/io/grpc/util/RoundRobinLoadBalancerFactory.java core/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java

ejona86 · 2018-08-07T17:48:07Z

Ping @zhangkun83

zhangkun83 · 2018-08-09T00:53:03Z

Thanks for the patience @njhill. I am not excited about the added complexity. I would prefer doing the picker comparison.

zhangkun83 · 2018-08-14T17:43:58Z

@njhill, are you still working on this?

njhill · 2018-08-16T23:40:44Z

@zhangkun83 yes, apologies I was out travelling for a couple of days and bit behind with things, I will get back to it soon hopefully (if not this week then next week).

zhangkun83 · 2018-08-17T00:05:45Z

No worries. Just checking the status on your side :-)

Conflicts: core/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java

njhill · 2018-08-26T19:58:33Z

@zhangkun83 I've now updated this to save the current picker but instead of doing a full re-generation and comparison of the READY subchannel list every time, it uses a readily-accessible determination of whether the list has changed or not. I've added some comments and restructured a little to make this clearer.

It also more cleanly separates the different cases of a "ready" picker, an "empty" picker (which may or may not have an associated error), and a name resolution error. For example there is no need to touch the picker on name resolution errors as is currently done.

Please let me know what you think!

zhangkun83

I am fine with dividing the picker into two, but I still feel strong for creating and comparing the pickers. See my inline comments for my reasoning.

As for how to compare, we probably don't want to override equals() because the picker is stateful. Instead, I would create a util method areEquivalentPickers().

zhangkun83 · 2018-08-28T18:36:29Z

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

+      if (!(currentPicker instanceof ReadyPicker)) {
+        currentPicker = new EmptyPicker(error);
+      }
+      helper.updateBalancingState(TRANSIENT_FAILURE, currentPicker);


It's arguable whether a previously READY channel should turn TRANSIENT_FAILURE when receiving a name resolution error. Either way, the state should be consistent with the type of the picker.

If you want to keep the current ready picker, the state should also be READY too -- as this would be another behavior change, please do it in a separate PR.

@zhangkun83 unless I've made a mistake, this PR should contain zero behaviour change (apart from eliminating unnecessary shuffles of the picker index of course).

In particular the logic here is what the existing logic actually does if you unravel it. I.m.o. it's better for it to be explicit so that it's easier to reason about and decide whether the behaviour is appropriate (which I didn't think too much about).

Specifically in the existing code:

The ConnectivityState (here TRANSIENT_FAILURE) is passed to updateBalancingState unconditionally, and isn't part of the generated Picker's state at all

Picker completely ignores error if it has a non-empty list of (READY) subchannels

The name resolution error event by itself has no effect on the set of READY subchannels and therefore no effect on the generated picker unless it's already empty

You are right about that the behavior is unchanged. My mistake.

zhangkun83 · 2018-08-28T19:40:17Z

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

      }

-      updateBalancingState(getAggregatedState(), getAggregatedError());
+      updateBalancingState(activeListChanged || stickinessStateChanged);


While the logic is clear enough as it is, I still don't like the little addition of complexity. The list change handling is not in the RPC critical path, and the overhead of creating and discarding a new picker doesn't seem to be significant enough to justify the added code complexity, e.g., the boolean argument here passes ad-hoc information across function boundaries just because your logic has to be divided into multiple functions instead of being one -- not saying it's wrong, but I'd like to avoid such situation if possible.

Sure, I will rework to always re-generate and compare the pickers.

njhill · 2018-08-30T23:06:59Z

@zhangkun83 now updated to do picker comparison, PTAL at your convenience!

zhangkun83 · 2018-08-31T23:44:07Z

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

+      }
+    }
+
+    private static boolean areEquivalentPickers(SubchannelPicker p1, SubchannelPicker p2) {


There are uncovered lines. You may want to expose it as VisibleForTesting and have it thoroughly tested.

I refactored this in the latest commit, moving the logic to a new private abstract class method RoundRobinPicker.isEquivalentTo() (to help make it clearer that it only applies to the private RRLB pickers and not SubchannelPickers in general).

zhangkun83 · 2018-08-31T23:50:46Z

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

+        // initialize the Picker to a random start index to ensure that a high frequency of Picker
+        // churn does not skew subchannel selection.
+        int startIndex = random.nextInt(activeList.size());
+        updateBalancingState(READY, new ReadyPicker(activeList, startIndex, stickinessState));


You never call getAggregatedState() when the state is READY. This shadows the return READY branch in that method (noticed by coveralls). Since getAggregatedState() is only called once now, probably better to move it into updateBalancingState().

I've now combined/collapsed both the getAggregatedState() and getAggregateError() methods into updateBalancingState().

zhangkun83 · 2018-08-31T23:55:20Z

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

+      if (stateInfoRef.value.getState() == SHUTDOWN) {
+        // This is the case the shutdown was triggered by a name resolver removal, the channel
+        // shutdown state change logic was already triggered in handleResolvedAddressGroups().
+        return;


This new branch is not covered in test. It doesn't seem necessary either, because the next branch doesn't change anything, and the following branch will just return.

zhangkun83 · 2018-09-01T00:11:01Z

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

+      getSubchannelStateInfoRef(subchannel).value =
+          ConnectivityStateInfo.forNonError(SHUTDOWN);
+      if (stickinessState != null) {
+        stickinessState.remove(subchannel);


This branch is not covered.

I've extended a unit test to cover this case - name resolver removal of a sticky subchannel.

zhangkun83 · 2018-09-01T00:12:43Z

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

+        return false;
+      }
+      if (p1 instanceof EmptyPicker) {
+        return Objects.equal(((EmptyPicker)p1).status, ((EmptyPicker)p2).status);


Please add a space following the closing parenthesis of class casting.

- remove now redundant check in handleSubchannelState - collapse getAggregatedState() and getAggregatedError() into handleBalancingState() - have both pickers extend new RoundRobinPicker, move areEquivalentPickers() logic into RoundRobinPicker.isEquivalentTo() - extend unit tests to cover some additional cases

njhill · 2018-09-04T08:05:42Z

@zhangkun83 thanks for the detailed review. Have made some updates which hopefully address your comments. One of the builds failed but I don't think it was code related.

zhangkun83 · 2018-09-05T18:09:40Z

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

+    private final Status status;
+
+    EmptyPicker(@Nullable Status status) {
+      this.status = status;


Making it non-Nullable, and using OK to indicate PickResult.withNoResult() may simplify the code a little bit.

zhangkun83 · 2018-09-05T18:13:45Z

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

-        List<Subchannel> list, @Nullable Status status, int startIndex,
+    ReadyPicker(List<Subchannel> list, int startIndex,
        @Nullable RoundRobinLoadBalancer.StickinessState stickinessState) {
+      assert !list.isEmpty();


This should be a checkArgument(). If this did happen in production and it was let pass this point, it would cause an out-of-bound exception at a later point.

zhangkun83 · 2018-09-05T18:16:13Z

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

+    @Override
+    boolean isEquivalentTo(RoundRobinPicker picker) {
+      if (!(picker instanceof ReadyPicker)) {
+        return false;


This is not covered.

- Use explicit check for non-empty list instead of assert - Change EmptyPicker.status to be non-nullable - Further test coverage improvement including explicit picker comparison tests

njhill · 2018-09-06T19:52:04Z

@zhangkun83 I've addressed your latest comments, PTAL. In particular there are now no new uncovered lines. Thanks!

zhangkun83

LGTM except for a minor comment.

zhangkun83 · 2018-09-07T23:54:28Z

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

    private final Random random;

+    private ConnectivityState currentState;
+    private RoundRobinPicker currentPicker = new EmptyPicker(Status.OK);


Use EMPTY_OK

njhill · 2018-09-08T10:33:44Z

Thanks @zhangkun83, have now addressed your remaining minor comment.

zhangkun83 · 2018-09-09T06:06:12Z

@carl-mastrangelo do you want to take another look?

carl-mastrangelo

LGTM

Some style nits but i don't want to block this.

carl-mastrangelo · 2018-09-12T22:07:36Z

@njhill Merged, thanks!

Sorry for the excessively long review process! Hopefully this hasn't scared you off; we do appreciate your contributions. This will be released in 1.16 in about 6 weeks.

dapengzhang0 assigned zhangkun83 Jul 2, 2018

carl-mastrangelo requested a review from dapengzhang0 July 13, 2018 18:10

carl-mastrangelo reviewed Jul 13, 2018

View reviewed changes

remove explicit initialization of boolean ready field

1c9c0f6

per @carl-mastrangelo's review comment

carl-mastrangelo reviewed Jul 31, 2018

View reviewed changes

zhangkun83 reviewed Aug 1, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into rrlb-less-churn

36d83f3

Conflicts: core/src/main/java/io/grpc/util/RoundRobinLoadBalancerFactory.java core/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java

njhill force-pushed the rrlb-less-churn branch from f46c60d to 36d83f3 Compare August 2, 2018 06:34

njhill added 3 commits August 26, 2018 08:14

minor restructuring to make logic clearer; more explanatory comments

a693c30

Merge remote-tracking branch 'upstream/master' into rrlb-less-churn

d1d02c0

Conflicts: core/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java

move some checks inside updateBalancingState method for clarity

3cbb05d

zhangkun83 reviewed Aug 28, 2018

View reviewed changes

store current state and picker in RRLB, only update when new one is diff

7d9bde0

zhangkun83 requested changes Sep 1, 2018

View reviewed changes

njhill force-pushed the rrlb-less-churn branch from 0153447 to b1a7935 Compare September 1, 2018 20:46

zhangkun83 reviewed Sep 5, 2018

View reviewed changes

Address latest review comments from @zhangkun83

b902bc2

- Use explicit check for non-empty list instead of assert - Change EmptyPicker.status to be non-nullable - Further test coverage improvement including explicit picker comparison tests

zhangkun83 added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Sep 8, 2018

kokoro-team removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Sep 8, 2018

zhangkun83 approved these changes Sep 8, 2018

View reviewed changes

use EMPTY_OK instead of Status.OK for initial empty picker

3df0d18

zhangkun83 added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Sep 8, 2018

kokoro-team removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Sep 8, 2018

carl-mastrangelo approved these changes Sep 12, 2018

View reviewed changes

carl-mastrangelo merged commit ed709ff into grpc:master Sep 12, 2018

njhill deleted the rrlb-less-churn branch September 13, 2018 13:19

lock bot locked as resolved and limited conversation to collaborators Dec 12, 2018

Remove redundant SubchannelPicker refreshes in RoundRobinLoadBalancer #4607

Remove redundant SubchannelPicker refreshes in RoundRobinLoadBalancer #4607

Uh oh!

Conversation

njhill commented Jul 1, 2018

Uh oh!

njhill commented Jul 12, 2018

Uh oh!

carl-mastrangelo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

njhill commented Jul 13, 2018

Uh oh!

njhill commented Jul 29, 2018

Uh oh!

carl-mastrangelo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhangkun83 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

njhill commented Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ejona86 commented Aug 7, 2018

Uh oh!

zhangkun83 commented Aug 9, 2018

Uh oh!

zhangkun83 commented Aug 14, 2018

Uh oh!

njhill commented Aug 16, 2018

Uh oh!

zhangkun83 commented Aug 17, 2018

Uh oh!

njhill commented Aug 26, 2018

Uh oh!

zhangkun83 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

njhill Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

njhill commented Aug 1, 2018 •

edited

Loading

njhill Aug 28, 2018 •

edited

Loading