{"action":"created","issue":{"url":"https://api.github.com/repos/grpc/grpc-java/issues/2716","repository_url":"https://api.github.com/repos/grpc/grpc-java","labels_url":"https://api.github.com/repos/grpc/grpc-java/issues/2716/labels{/name}","comments_url":"https://api.github.com/repos/grpc/grpc-java/issues/2716/comments","events_url":"https://api.github.com/repos/grpc/grpc-java/issues/2716/events","html_url":"https://github.com/grpc/grpc-java/issues/2716","id":206718953,"number":2716,"title":"Unify usage of EAG and ResolvedServer","user":{"login":"lukaszx0","id":11562,"avatar_url":"https://avatars.githubusercontent.com/u/11562?v=3","gravatar_id":"","url":"https://api.github.com/users/lukaszx0","html_url":"https://github.com/lukaszx0","followers_url":"https://api.github.com/users/lukaszx0/followers","following_url":"https://api.github.com/users/lukaszx0/following{/other_user}","gists_url":"https://api.github.com/users/lukaszx0/gists{/gist_id}","starred_url":"https://api.github.com/users/lukaszx0/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lukaszx0/subscriptions","organizations_url":"https://api.github.com/users/lukaszx0/orgs","repos_url":"https://api.github.com/users/lukaszx0/repos","events_url":"https://api.github.com/users/lukaszx0/events{/privacy}","received_events_url":"https://api.github.com/users/lukaszx0/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2017-02-10T06:28:24Z","updated_at":"2017-02-12T08:53:20Z","closed_at":null,"body":"@zhangkun83 [wrote](https://github.com/grpc/grpc-java/pull/2696#issuecomment-277759343):\r\n\r\n> 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.\r\n> \r\n> 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.\r\n> \r\n> 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."},"comment":{"url":"https://api.github.com/repos/grpc/grpc-java/issues/comments/279205102","html_url":"https://github.com/grpc/grpc-java/issues/2716#issuecomment-279205102","issue_url":"https://api.github.com/repos/grpc/grpc-java/issues/2716","id":279205102,"user":{"login":"lukaszx0","id":11562,"avatar_url":"https://avatars.githubusercontent.com/u/11562?v=3","gravatar_id":"","url":"https://api.github.com/users/lukaszx0","html_url":"https://github.com/lukaszx0","followers_url":"https://api.github.com/users/lukaszx0/followers","following_url":"https://api.github.com/users/lukaszx0/following{/other_user}","gists_url":"https://api.github.com/users/lukaszx0/gists{/gist_id}","starred_url":"https://api.github.com/users/lukaszx0/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lukaszx0/subscriptions","organizations_url":"https://api.github.com/users/lukaszx0/orgs","repos_url":"https://api.github.com/users/lukaszx0/repos","events_url":"https://api.github.com/users/lukaszx0/events{/privacy}","received_events_url":"https://api.github.com/users/lukaszx0/received_events","type":"User","site_admin":false},"created_at":"2017-02-12T08:53:20Z","updated_at":"2017-02-12T08:53:20Z","body":"> 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.\r\n\r\nAgreed. It absolutely makes sense to have it as flat as possible and let users to augment it with `Attributes`.\r\n\r\n> 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.\r\n\r\nYes, but for common things like calculating [new/removed servers it's still quite inconvenient](https://github.com/grpc/grpc-java/blob/master/core/src/main/java/io/grpc/util/RoundRobinLoadBalancerFactory2.java#L113), to do this conversion.\r\n\r\nSo 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).\r\n\r\nWhat do you think about making following changes:\r\n\r\n- Switch from `ResolvedServerInfo` to `EquivalentAddressGroup` in NameResolver.\r\n- Rename `ResolvedServerGroup` to `ServerInfo` (Or something else generic. Minor detail).\r\n- Change NameResolver API to `onUpdate(List<ServerInfo>, Attributes)`.\r\n- Change LB API to `createSubchannel(ServerInfo, Attributes)`.\r\n\r\n1. NameResolver & LoadBalancer APIs will be consistent and will _match_.\r\n2. NameResolver API will, in my opinion, be less confusing.\r\n\r\nRegarding 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])`\r\n/ `ServerInfo.for(List<SocketAddr>[, Attributes])`.\r\n\r\n@zhangkun83 thoughts?"}}
{"actor":{"display_login":"lukaszx0"}}