Change PickFirstLeafLoadBalancer to only have 1 subchannel at a time #11520

larry-safran · 2024-09-12T01:47:51Z

Hide behind environment variable GRPC_SERIALIZE_RETRIES

This is for testing with GMS core to see if they use the new PF with only 1 subchannel at a time trying to reconnect whether that eliminates their 6% data increase.

ejona86

Sending what I have. I'll need to look at this more later.

core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java

ejona86 · 2024-09-23T21:01:46Z

core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java

@@ -458,7 +538,8 @@ private SubchannelData createNewSubchannel(SocketAddress addr, Attributes attrs)
  }

  private boolean isPassComplete() {
-    if (addressIndex.isValid() || subchannels.size() < addressIndex.size()) {
+    if ((!serializingRetries && addressIndex.isValid())


Why do we want to ignore whether the index is valid?

Because we reset it for subchannel retry.

This is still nagging at me. I feel like either we never need this condition or we need to replace it with something else for serializingRetries. I agree that we need this function to return true even though we've reset the index (for ++numTf).

You're right, there would never be a time where that was true but the subchannels.size() < addressIndex.size() was false. We do have the firstPass variable, so I used that where this is called to skip the comparisons on the repeated checks once we've seen the end.

core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java

…if environment variable GRPC_SERIALIZE_RETRIES == true. Cache serializingRetries value so that it doesn't have to look up the flag every time. Clear the correct task when READY in processSubchannelState and move the logic to cancelScheduledTasks Cleanup based on PR review remove unneeded checks for shutdown.

…trol of retries.

larry-safran · 2024-09-26T01:31:41Z

Ready for re-review

ejona86

Sending what I have. This seems much less bug-prone

core/src/main/java/io/grpc/internal/InternalSubchannel.java

api/src/main/java/io/grpc/LoadBalancer.java

core/src/main/java/io/grpc/internal/InternalSubchannel.java

core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java

ejona86

After the first failure for an address, I don't see how we start a second connection to the same address (after the backoff).

…t is disabled. Remove an extra index.increment in LeafLB Fix spelling, remove unneeded additions

ejona86

Looks good other than the nagging !serializingRetries && addressIndex.isValid()

core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java

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

ejona86 · 2024-09-30T22:58:43Z

core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java

@@ -458,7 +538,8 @@ private SubchannelData createNewSubchannel(SocketAddress addr, Attributes attrs)
  }

  private boolean isPassComplete() {
-    if (addressIndex.isValid() || subchannels.size() < addressIndex.size()) {
+    if ((!serializingRetries && addressIndex.isValid())


This is still nagging at me. I feel like either we never need this condition or we need to replace it with something else for serializingRetries. I agree that we need this function to return true even though we've reset the index (for ++numTf).

ejona86 · 2024-10-02T19:49:06Z

core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java

          }
        }

-        if (isPassComplete()) {
+        if (!firstPass || isPassComplete()) {


Let's at least add a comment that the !firstPass is an optimization.

Just removed it. Looping on a boolean comparison hopefully won't take too long.

…rpc#11520) * Change PickFirstLeafLoadBalancer to only have 1 subchannel at a time if environment variable GRPC_SERIALIZE_RETRIES == true. Cache serializingRetries value so that it doesn't have to look up the flag every time. Clear the correct task when READY in processSubchannelState and move the logic to cancelScheduledTasks Cleanup based on PR review remove unneeded checks for shutdown. * Fix previously broken tests * Shutdown previous subchannel when run off end of index. * Provide option to disable subchannel retries to let PFLeafLB take control of retries. * InternalSubchannel internally goes to IDLE when sees TF when reconnect is disabled. Remove an extra index.increment in LeafLB

larry-safran requested a review from ejona86 September 12, 2024 01:47

ejona86 reviewed Sep 12, 2024

View reviewed changes

core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java Outdated Show resolved Hide resolved

core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java Show resolved Hide resolved

ejona86 reviewed Sep 23, 2024

View reviewed changes

larry-safran force-pushed the new_pf_prob branch 2 times, most recently from 289ca19 to df8e78c Compare September 24, 2024 19:30

ejona86 reviewed Sep 24, 2024

View reviewed changes

core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java Outdated Show resolved Hide resolved

larry-safran added 5 commits September 25, 2024 17:44

Fix previously broken tests

2658447

Fix previously broken tests

bd5f66f

Shutdown previous subchannel when run off end of index.

bced263

Provide option to disable subchannel retries to let PFLeafLB take con…

3f910b0

…trol of retries.

larry-safran force-pushed the new_pf_prob branch from d15a3ee to 3f910b0 Compare September 26, 2024 00:45

larry-safran requested a review from ejona86 September 26, 2024 01:31

ejona86 reviewed Sep 26, 2024

View reviewed changes

larry-safran added 3 commits September 27, 2024 16:30

InternalSubchannel internally goes to IDLE when sees TF when reconnec…

f21b749

…t is disabled. Remove an extra index.increment in LeafLB Fix spelling, remove unneeded additions

Make errorprone happy

d50acf8

Make checkstyle happy

0b6e846

larry-safran requested a review from ejona86 September 28, 2024 01:16

Fix the other spelling mistake

b1ff88d

ejona86 reviewed Sep 30, 2024

View reviewed changes

Cleanup

e854c20

ejona86 approved these changes Oct 2, 2024

View reviewed changes

Remove unneeded optimization

a28b548

larry-safran merged commit 9bb06af into grpc:master Oct 3, 2024
15 checks passed

larry-safran deleted the new_pf_prob branch October 3, 2024 00:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change PickFirstLeafLoadBalancer to only have 1 subchannel at a time #11520

Change PickFirstLeafLoadBalancer to only have 1 subchannel at a time #11520

larry-safran commented Sep 12, 2024

ejona86 left a comment

ejona86 Sep 23, 2024

larry-safran Sep 26, 2024

ejona86 Sep 30, 2024

larry-safran Sep 30, 2024

larry-safran commented Sep 26, 2024

ejona86 left a comment

ejona86 left a comment

ejona86 left a comment

ejona86 Sep 30, 2024

ejona86 Oct 2, 2024

larry-safran Oct 2, 2024

Change PickFirstLeafLoadBalancer to only have 1 subchannel at a time #11520

Change PickFirstLeafLoadBalancer to only have 1 subchannel at a time #11520

Conversation

larry-safran commented Sep 12, 2024

ejona86 left a comment

Choose a reason for hiding this comment

ejona86 Sep 23, 2024

Choose a reason for hiding this comment

larry-safran Sep 26, 2024

Choose a reason for hiding this comment

ejona86 Sep 30, 2024

Choose a reason for hiding this comment

larry-safran Sep 30, 2024

Choose a reason for hiding this comment

larry-safran commented Sep 26, 2024

ejona86 left a comment

Choose a reason for hiding this comment

ejona86 left a comment

Choose a reason for hiding this comment

ejona86 left a comment

Choose a reason for hiding this comment

ejona86 Sep 30, 2024

Choose a reason for hiding this comment

ejona86 Oct 2, 2024

Choose a reason for hiding this comment

larry-safran Oct 2, 2024

Choose a reason for hiding this comment