search

grpc / grpc-java

The Java gRPC implementation. HTTP/2 based RPC
https://grpc.io/docs/languages/java/
Apache License 2.0
11.26k stars 3.79k forks source link

Unify usage of EAG and ResolvedServer #2716

Closed lukaszx0 closed 7 years ago

lukaszx0 commented 7 years ago

@zhangkun83 wrote:

The equality of EquivalentAddressGroup, which only counts the addresses but not the attributes, was needed by the v1 ManagedChannelImpl for de-duplicating TransportSets. Even though ManagedChannelImpl2 doesn't forbid duplicating InternalSubchannel for the same EquivalentAddressGroup, the equality is still needed by most LoadBalancer2 implementors, e.g., Map<EquivalentAddressGroup, Subchannel> subchannels in the round-robin LB, to determine which InternalSubchannels to close and which to keep, when the NR result is updated.

I agree that there are too many address wrappers and it has become very confusing, sometimes even to myself. I prefer to keep the EquivalentAddressGroup name, because it indicates how these addresses are used by the channel. So instead of deleting EquivalentAddressGroup, I suggest deleting ResolvedServerInfo, and adding attributes to EquivalentAddressGroup. To address the equality requirement I mentioned earlier, we could add a withoutAttributes() method which can be used in cases where attributes should not be counted for equality.

ResolvedServerInfoGroup was intended to be the attribute-ful counter-part of EquivalentAddressGroup. Since you are proposing to replace EquivalentAddressGroup with ResolvedServerInfo, I would expect ResolvedServerInfoGroup to go away instead of building yet another level on top of ResolvedServerInfo.

lukaszx0 commented 7 years ago

Even though ManagedChannelImpl2 doesn't forbid duplicating InternalSubchannel for the same EquivalentAddressGroup, the equality is still needed by most LoadBalancer2 implementors, e.g., Map<EquivalentAddressGroup, Subchannel> subchannels in the round-robin LB, to determine which InternalSubchannels to close and which to keep, when the NR result is updated.

Looks like I actually never understood API of NameResolver's onUpdate (even though IIRC I contributed parts of it, heh). I always thought and I treated it like it was meant to return a groups of hosts where by groups I understood clusters (ie. foobarapp-nyc cluster) containing a list of nodes of a given cluster. That's why it seemed somewhat natural to propose adding third level (foobardapp-nyc (cluster) => foobar.nyc.example.com (node) => [foobar.nyc.example.com, 10.0.0.123, ...] (equivalent addresses)). The docs for name resolver aren't very clear on this hence my misunderstanding.

I agree that there are too many address wrappers and it has become very confusing, sometimes even to myself. I prefer to keep the EquivalentAddressGroup name, because it indicates how these addresses are used by the channel. So instead of deleting EquivalentAddressGroup, I suggest deleting ResolvedServerInfo, and adding attributes to EquivalentAddressGroup. To address the equality requirement I mentioned earlier, we could add a withoutAttributes() method which can be used in cases where attributes should not be counted for equality.

Basically my main objective is to unify LB & NR API, by being able to pass address(es) received in handleResolvedAddresses directly to createSubchannel without need of conversion and weird dances.

ResolvedServerInfoGroup was intended to be the attribute-ful counter-part of EquivalentAddressGroup. Since you are proposing to replace EquivalentAddressGroup with ResolvedServerInfo, I would expect ResolvedServerInfoGroup to go away instead of building yet another level on top of ResolvedServerInfo.

That makes sense, though unfortunately it'll change and break NameResolver API again...

zhangkun83 commented 7 years ago

The grouping of addresses in NameResolver's onUpdate directly maps to EquivalentAddressGroup. I do feel it necessary to reduce the number of these classes to prevent more confusions.

I always thought and I treated it like it was meant to return a groups of hosts where by groups I understood clusters (ie. foobarapp-nyc cluster) containing a list of nodes of a given cluster. That's why it seemed somewhat natural to propose adding third level (foobardapp-nyc (cluster) => foobar.nyc.example.com (node) => [foobar.nyc.example.com, 10.0.0.123, ...] (equivalent addresses)).

A pre-defined hierarchy of the addresses doesn't only lack flexibility (what if the user's setup is structured even slightly different?), but also hard to comprehend when there are more than two layers. For you use case, I would instead attach cluster IDs to the attributes of each address group. More dimensions can be added by simply adding another attribute key.

Basically my main objective is to unify LB & NR API, by being able to pass address(es) received in handleResolvedAddresses directly to createSubchannel without need of conversion and weird dances.

ResolvedServerInfoGroup.toEquivalentAddressGroup() is already there to make the conversion easy. LB does need to look into the attributes most of the time, so I am not sure there is more need to be simplified.

That makes sense, though unfortunately it'll change and break NameResolver API again...

It may have looked more tidy if we didn't have ResolvedServerInfoGroup and ResolvedServerInfo, and had attributes in EquivalentAddressGroup instead, but the current API doesn't seem broken either. I am a little hesitant to make the change because I am not sure the benefit is worth the churn.

lukaszx0 commented 7 years ago

A pre-defined hierarchy of the addresses doesn't only lack flexibility (what if the user's setup is structured even slightly different?), but also hard to comprehend when there are more than two layers. For you use case, I would instead attach cluster IDs to the attributes of each address group. More dimensions can be added by simply adding another attribute key.

Agreed. It absolutely makes sense to have it as flat as possible and let users to augment it with Attributes.

ResolvedServerInfoGroup.toEquivalentAddressGroup() is already there to make the conversion easy. LB does need to look into the attributes most of the time, so I am not sure there is more need to be simplified.

Yes, but for common things like calculating new/removed servers it's still quite inconvenient, to do this conversion.

So generally, in my opinion, for most people the only case when the resolver will return multiple addresses for the same server is the canonical example of DNS. I think that most 3rd party implementations provided by users will just resolve address with one hostname (which will eventually be resolved by built in system java resolver).

What do you think about making following changes:

  1. NameResolver & LoadBalancer APIs will be consistent and will match.
  2. NameResolver API will, in my opinion, be less confusing.

Regarding 2), I've actually started wondering if we really need Attributes in EAG. Dropping them would let us make very simple API for ServerInfo like:ServerInfo.for(SocketAddr[, Attributes]) / ServerInfo.for(List<SocketAddr>[, Attributes]).

@zhangkun83 thoughts?

zhangkun83 commented 7 years ago

Switch from ResolvedServerInfo to EquivalentAddressGroup in NameResolver.

What do you mean by that?

So generally, in my opinion, for most people the only case when the resolver will return multiple addresses for the same server is the canonical example of DNS. I think that most 3rd party implementations provided by users will just resolve address with one hostname (which will eventually be resolved by built in system java resolver).

I haven't seen any third-party NameResolvers so I wouldn't say how many cases in which multiple-address-per-same-server is useful, but at least the widespread of IPv6-IPv4-hybrid networks naturally leads to the demand of such semantics in the NameResolver interface. Our in-house NameResolver is not DNS, and there has been discussions about IPv6 support which may require it to emit multiple addresses (IPv4 and IPv6) for the same server. Just for IPv6 support I would say NameResolver should have an explicit notation of multiple-address-per-same-server.

Change NameResolver API to onUpdate(List<ServerInfo>, Attributes)

ServerInfo is too generic. It doesn't say how many addresses are in there and what their relationship is, which means NameResolvers get to define their own semantics, which is an implementation detail, but LB has to know it in order to interpret the ServerInfos properly. On the other hand, EquivalentAddressGroup clearly defines the relationships of addresses. This helps decoupling NR and LB implementations.

Regarding 2), I've actually started wondering if we really need Attributes in EAG. Dropping them would let us make very simple API for ServerInfo

I see ServerInfo or ResolvedServerInfoGroup as a fork of EquivalentAddressGroup with attributes. If we add attributes to EquivalentAddressGroup, ResolvedServerInfoGroup should go away. I can't make a case for ResolverServerInfo (single address and attributes), so that might go away too.

zhangkun83 commented 7 years ago

@lukaszx0 I just made #2755. It may need some polishing but it illustrate my idea well. I'd like to hear your feedback.

lukaszx0 commented 7 years ago

@zhangkun83 nice, thank you! Sorry for procrastination on this - I had it on my todo but didn't get to it.