core: Add Attributes.Key for authority in EquivalentAddressGroup (#4469) #6126

enguerrand · 2019-09-05T17:13:00Z

This enables NameResolvers to dynamically provide authority for each
Subchannel

linux-foundation-easycla · 2019-09-05T17:13:02Z

One or more committers are not authorized under a signed CLA as indicated below. Please click here to be authorized.

❌ edr (fc2620d)

ejona86 · 2019-09-05T19:38:18Z

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

+   * The authority to be used when constructing {@link io.grpc.Channel}s to this this EquivalentAddressGroup.
+   */
+  @EquivalentAddressGroup.Attr
+  public static final Attributes.Key<String> ATTR_CHANNEL_AUTHORITY =


This should be a public API, not in internal. @zhangkun83, is the only place we have for things like this io.grpc.Grpc?

Yes, the last time I checked io.grpc.Grpc is the only place where these Attrs are defined (and they all are prefixed TRANSPORT_ATTR_) . I guess this can also be considered a TRANSPORT_ATTR_ ?

I would suggest we name it as ATTR_AUTHORITY_OVERRIDE and put it in EquivalentAddressGroup. TRANSPORT_ATTR_ is for pulling information out of transport, while this attribute is passing information to the transport, which is different. It's better to put the attributes close to where it's used. TRANSPORT_ATTR_ is in Grpc because there is no other public place where we can put them, since transport API is internal.

Makes sense. Agreed on all points.

Renamed the attribute as suggested by @zhangkun83 . I am assuming that

put it in EquivalentAddressGroup

means using the @EquivalentAddressGroup.Attr annotation as I already did?

No, I think he meant move the field to io.grpc.EquivalentAddressGroup, directly.

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

sanjaypujare · 2019-09-05T20:08:59Z

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

@@ -50,6 +50,13 @@
  public static final Attributes.Key<Boolean> ATTR_LB_PROVIDED_BACKEND =
      Attributes.Key.create("io.grpc.grpclb.lbProvidedBackend");

+  /**
+   * The authority to be used when constructing {@link io.grpc.Channel}s to this this EquivalentAddressGroup.


this this -> this

Also did you mean to say Subchannel instead of Channel (which makes sense given the main PR comment.

@sanjaypujare, I went for Channel as the Subchannel could not be referenced from this context. I changed this to Subchannel and removed the javadoc link.

zhangkun83 · 2019-09-05T21:17:24Z

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

+   */
+  @EquivalentAddressGroup.Attr
+  public static final Attributes.Key<String> ATTR_CHANNEL_AUTHORITY =
+      Attributes.Key.create("io.grpc.grpclb.channelAuthority");


Please rename the key string accordingly.

@zhangkun83. Done, although I am not entirely sure about the convention here.

ejona86 · 2019-09-06T15:14:54Z

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

+
+    // Update reused subchannels with possibly updated EquivalentAddressGroups
+    for (EquivalentAddressGroup addressGroup : unchangedAddrs) {
+      subchannels.get(addressGroup).updateAddresses(resolvedAddresses.getAddresses());


No, no. This updates each subchannel to point to all addresses. It should be closer to:

subchannels.get(addressGroup).updateAddresses(addressGroup);

UpdateAddresses changes the addresses that the subchannel connects to, while leaving any connection in-place if the addresses overlap. In this case, the addresses should be identical, however the attributes may have changed. So we're using it to just swap to the new attributes.

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

ejona86 · 2019-09-06T15:23:39Z

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

+            setsDifference(currentAddrs.keySet(), latestAddrs.keySet());
+    Set<EquivalentAddressGroup> unchangedAddrsStripped =
+            setsDifference(currentAddrs.keySet(), removedAddrsStripped);
+    Set<EquivalentAddressGroup> addedAddrs =


Part of the point of stripping the Attributes was to avoid equals/hashCode of Attributes. This is using equals/HashCode. You should use the stripped versions of the set in loops, and then use latestAddrs.get() to pass the full-featured EAG to subchannel.

I refactored this to get rid of the stripping entirely (as I suggested earlier).
For this to be possible I had to make the "setsDifference" method ungeneric. I personally think this makes the code a lot easier to read but please let me know what you think.
I also added a unit test to validate the new diffing.

ejona86

We're still having trouble getting on the same page with RR, so I've sent out a PR to fix it up. That should make this PR much simpler, since that was where most of the issues were coming from.

ejona86 · 2019-09-09T18:23:16Z

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

 import io.grpc.Metadata;
 import io.grpc.Metadata.Key;
 import io.grpc.NameResolver;
 import io.grpc.Status;
 import io.grpc.internal.GrpcAttributes;
 import io.grpc.internal.ServiceConfigUtil;
+


Our style doesn't put blank lines between these imports.

ejona86 · 2019-09-09T18:25:22Z

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

-    Set<EquivalentAddressGroup> latestAddrs = stripAttrs(servers);
-    Set<EquivalentAddressGroup> addedAddrs = setsDifference(latestAddrs, currentAddrs);
-    Set<EquivalentAddressGroup> removedAddrs = setsDifference(currentAddrs, latestAddrs);
+    Set<EquivalentAddressGroup> latestAddrs = new HashSet<>(servers);


This still uses the equals() from Attributes, because EAG uses the equals() from attributes. I'll go ahead and fix up RR in a separate PR (#6136).

Ok, thanks for helping out. Should I squash my commits and discard the RR-related changes?

Yes, please do.

Ideally there would also be a test. I think copying InternalSubchannelTest.eagAttribute_propagatesToTransport would work well. You'd change attr to include ATTR_AUTHORITY_OVERRIDE and change the line with createClientTransportOptions() to set the authority.

This enables NameResolvers to dynamically provide authority for each Subchannel

ejona86

I just realized the experimental API annotation was missing. Since this is really close now, creating the issue now seems good.

(Approving, since this is also something I could do behind you, after merging this PR.)

ejona86 · 2019-09-10T12:31:39Z

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

+  /**
+   * The authority to be used when constructing Subchannels for this EquivalentAddressGroup.
+   */
+  @Attr


Add an @ExperimentalApi annotation here. Create a new issue to stabilize the API, and include it as the value. You can look at other experimental apis to see the format.

…UTHORITY_OVERRIDE

dapengzhang0 · 2019-09-11T19:56:25Z

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

+  @Attr
+  @ExperimentalApi("https://github.com/grpc/grpc-java/issues/6138")
+  public static final Attributes.Key<String> ATTR_AUTHORITY_OVERRIDE =
+      Attributes.Key.create("io.grpc.grpclb.authorityOverride");


I think this is not related to io.grpc.grpclb. Maybe io.grpc.EquivalentAddressGroup.authorityOverride is a better name.

Done. I went ahead and made this change, since it was trivial.

ejona86 · 2019-09-12T03:39:36Z

@enguerrand, thank you!

This did not make the branch cut (happened yesterday), which means it would be included in the release-after-next, in 2 months. How much of an issue would that be for you?

enguerrand · 2019-09-12T07:05:00Z

@ejona86, glad to see the changes were merged. If would certainly have helped me to have the changes in the upcoming release but I understand that you have processes to follow...

enguerrand mentioned this pull request Sep 5, 2019

Provide a way for NameResolvers to dynamically provide authority #4469

Closed

ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Sep 5, 2019

grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Sep 5, 2019

ejona86 reviewed Sep 5, 2019

View reviewed changes

ejona86 requested a review from zhangkun83 September 5, 2019 19:40

sanjaypujare reviewed Sep 5, 2019

View reviewed changes

zhangkun83 reviewed Sep 5, 2019

View reviewed changes

ejona86 reviewed Sep 6, 2019

View reviewed changes

ejona86 reviewed Sep 9, 2019

View reviewed changes

core: Add Attributes.Key for authority in EquivalentAddressGroup (#4469)

c2d3017

This enables NameResolvers to dynamically provide authority for each Subchannel

ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Sep 10, 2019

grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Sep 10, 2019

ejona86 approved these changes Sep 10, 2019

View reviewed changes

core: Add @experimentalapi reference to EquivalentAddressGroup.ATTR_A…

ffd5240

…UTHORITY_OVERRIDE

ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Sep 10, 2019

grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Sep 10, 2019

ejona86 requested a review from zhangkun83 September 10, 2019 14:54

ejona86 approved these changes Sep 10, 2019

View reviewed changes

ejona86 requested review from dapengzhang0 and removed request for zhangkun83 September 10, 2019 17:15

dapengzhang0 reviewed Sep 11, 2019

View reviewed changes

s/grpclb/EquivalentAddressGroup/

a4bcea1

dapengzhang0 approved these changes Sep 11, 2019

View reviewed changes

dapengzhang0 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Sep 11, 2019

grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Sep 11, 2019

ejona86 merged commit 132e8bc into grpc:master Sep 12, 2019

ejona86 mentioned this pull request Sep 26, 2019

Use a custom authority for each connection in a single ClientConn grpc/grpc-go#3038

Closed

lock bot locked as resolved and limited conversation to collaborators Dec 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: Add Attributes.Key for authority in EquivalentAddressGroup (#4469) #6126

core: Add Attributes.Key for authority in EquivalentAddressGroup (#4469) #6126

enguerrand commented Sep 5, 2019 •

edited by ejona86

Loading

linux-foundation-easycla bot commented Sep 5, 2019

ejona86 Sep 5, 2019

sanjaypujare Sep 5, 2019

zhangkun83 Sep 5, 2019

sanjaypujare Sep 5, 2019

enguerrand Sep 6, 2019

ejona86 Sep 6, 2019

enguerrand Sep 9, 2019

sanjaypujare Sep 5, 2019

enguerrand Sep 6, 2019 •

edited

Loading

zhangkun83 Sep 5, 2019

enguerrand Sep 6, 2019

ejona86 Sep 6, 2019 •

edited

Loading

enguerrand Sep 9, 2019

ejona86 Sep 6, 2019

enguerrand Sep 9, 2019

ejona86 left a comment

ejona86 Sep 9, 2019

ejona86 Sep 9, 2019

enguerrand Sep 10, 2019

ejona86 Sep 10, 2019

ejona86 left a comment

ejona86 Sep 10, 2019

enguerrand Sep 10, 2019

dapengzhang0 Sep 11, 2019

ejona86 Sep 11, 2019

ejona86 commented Sep 12, 2019

enguerrand commented Sep 12, 2019

core: Add Attributes.Key for authority in EquivalentAddressGroup (#4469) #6126

core: Add Attributes.Key for authority in EquivalentAddressGroup (#4469) #6126

Conversation

enguerrand commented Sep 5, 2019 • edited by ejona86 Loading

linux-foundation-easycla bot commented Sep 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enguerrand Sep 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejona86 Sep 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejona86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejona86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejona86 commented Sep 12, 2019

enguerrand commented Sep 12, 2019

enguerrand commented Sep 5, 2019 •

edited by ejona86

Loading

enguerrand Sep 6, 2019 •

edited

Loading

ejona86 Sep 6, 2019 •

edited

Loading