xds: Fix load reporting when pick first is used for locality-routing. by DNVindhya · Pull Request #11495 · grpc/grpc-java

DNVindhya · 2024-08-28T21:55:02Z

This fixes Incorrect load reporting when using pick first as locality-routing policy.

Cause: ClusterImplLoadBalancer used to assume all the address are in the same locality and used first address in the list of addresses to determine locality. This is not always true leading to incorrect load reporting.

Fix: Get locality from address connected by the subchannel.

Closes #11434.

…mption that all addresses for a subchannel are in the same locality.

larry-safran

I've recommended a number of minor changes to comments

larry-safran · 2024-08-29T00:32:43Z

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

+/**
+ * An internal class. Do not use.
+ *
+ * <p>An interface to provide the address connected by subchannel.


This says it provides the address, but the method provides address attributes.

Updated the interface name to InternalSubchannelAddressAttributes.

larry-safran · 2024-08-29T00:39:11Z

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

        public void shutdown() {
-          if (localityStats != null) {
-            localityStats.release();
+          if (localityAtomicReference.get() != null) {


This can't possibly be null AFAICT

You are right. Removed the null check.

larry-safran · 2024-08-29T00:44:24Z

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

+      // attributes with its locality, including endpoints in LOGICAL_DNS clusters.
+      // In case of not (which really shouldn't), loads are aggregated under an empty
+      // locality.
+      if (locality == null) {


It shouldn't be, but is possible that locality is set but locality name is null. You should probably have an else clause that does a null check on localityName.

This is existing code, and it seems in general code assumes the values to be non-null. I'd much rather we handle that centrally in io.grpc.xds.client.Locality.create() than lots of random not-possible-to-trigger checks. The data is primarily coming from a proto, so it will be "" when unset.

Are you suggesting to move the locality name null check to io.grpc.xds.client.Locality.create()?

Oh, I see now Larry's comment wasn't about the Locality struct, but instead the attribute. That had been discussed when the code was originally introduced:
#11133 (comment)

This PR is likely to be reverted (in some way) later, once the old PF policy goes away. So changes to the existing code are likely to be lost.

larry-safran · 2024-08-29T00:45:51Z

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

  }
+
+  /**
+   * Represents the locality attributes of a cluster.


It is actually the stats and the name. Name could be considered an attribute, the the stats aren't.

Updated. PTAL

larry-safran · 2024-08-29T00:46:49Z

xds/src/main/java/io/grpc/xds/client/LoadStatsManager2.java

    checkState(allDropStats.containsKey(cluster)
            && allDropStats.get(cluster).containsKey(edsServiceName),
-        "stats for cluster %s, edsServiceName %s not exits", cluster, edsServiceName);
+        "stats for cluster %s, edsServiceName %s not exist", cluster, edsServiceName);


s/not exist/do not exist/

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

ejona86 · 2024-08-29T16:49:39Z

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

+      // attributes with its locality, including endpoints in LOGICAL_DNS clusters.
+      // In case of not (which really shouldn't), loads are aggregated under an empty
+      // locality.
+      if (locality == null) {


This is existing code, and it seems in general code assumes the values to be non-null. I'd much rather we handle that centrally in io.grpc.xds.client.Locality.create() than lots of random not-possible-to-trigger checks. The data is primarily coming from a proto, so it will be "" when unset.

ejona86 · 2024-08-30T14:14:49Z

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

-
+      // This clusterLocality ideally should not be utilized. We derive locality
+      // information from the first address, even prior to the subchannel becoming ready.
+      // This is primarily for the purpose of facilitating load reporting in the pre-READY


There's no load reporting pre-READY. But an LB API could return the subchannel even before it is ready, and PF does this. We generally won't end up reporting load for such picks because the channel will ignore the selected (not-ready) subchannel, but we needed to handle the case.

I see, updated the comment.

Did you miss pushing the update?

Discussed offline, we both are able to see the updated comment.

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

…ITY set.

larry-safran · 2024-08-30T21:30:55Z

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

+  private static final Attributes.Key<AtomicReference<ClusterLocality>>
+      ATTR_CLUSTER_LOCALITY =


You can put ATTR_CLUSTER_LOCALITY on the same line as the type.

Determine subchannel locality from connected address, discarding assu…

169537c

…mption that all addresses for a subchannel are in the same locality.

DNVindhya requested review from ejona86 and larry-safran August 28, 2024 21:55

fix unit test: get updated PickReslt after locality is updated

abaf93d

larry-safran approved these changes Aug 29, 2024

View reviewed changes

ejona86 reviewed Aug 29, 2024

View reviewed changes

addressed review comments

3a08e3f

ejona86 approved these changes Aug 30, 2024

View reviewed changes

Add check for subchannel was used that didn't have ATTR_CLUSTER_LOCAL…

885bf99

…ITY set.

larry-safran reviewed Aug 30, 2024

View reviewed changes

DNVindhya added 2 commits August 30, 2024 14:42

Addressed larry-safran's comment on formatting

0a2e1fb

fix typo

60fb499

DNVindhya merged commit 1dae144 into grpc:master Aug 31, 2024

DNVindhya deleted the pick_first_load_report branch August 31, 2024 23:07

github-actions bot locked as resolved and limited conversation to collaborators Nov 30, 2024

		private static final Attributes.Key<AtomicReference<ClusterLocality>>
		ATTR_CLUSTER_LOCALITY =

Comments

Conversation

DNVindhya commented Aug 28, 2024

Uh oh!

larry-safran 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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants