util: dual stack outlier detection change - #10939
Conversation
bb0988d to
ebb630d
Compare
ebb630d to
72b2f9b
Compare
temawi
left a comment
temawi
left a comment
There was a problem hiding this comment.
Looks pretty good. Left you some comments to think about.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
| Map<SocketAddress, Set<SocketAddress>> addressEndpointMap = new HashMap<>(); | ||
| for (EquivalentAddressGroup addressGroup : resolvedAddresses.getAddresses()) { | ||
| addresses.addAll(addressGroup.getAddresses()); | ||
| Set<SocketAddress> endpoint = ImmutableSet.copyOf(addressGroup.getAddresses()); |
There was a problem hiding this comment.
It would be better to use MultiChannelLoadBalancer.Endpoint which is optimized for comparison rather than the Set.
There was a problem hiding this comment.
Here the use case actually does not use comparison/look up much. The endpoint look up only happen in the address update, which does not happen so often already. Most computation is RPC counting for doing outlier detection algorithm, and in that case the subchannel and tracker already have references with each other that does not need endpoint look up that might need computing hash code. So I just used unordered set as called out from the grfc. Also a simple set saves sorting at the MultiChannelLoadBalancer.Endpoint construction time.
| Set<SocketAddress> endpoint = ImmutableSet.copyOf(addressGroup.getAddresses()); | ||
| endpoints.add(endpoint); | ||
| for (SocketAddress address : addressGroup.getAddresses()) { | ||
| addressEndpointMap.putIfAbsent(address, endpoint); |
There was a problem hiding this comment.
It is invalid for an address to be in multiple endpoints, you should at least log a warning if not outright reject the configuration.
There was a problem hiding this comment.
I just changed it to put and logged - I don't want to do any sophisticated validation that rejects the configuration here. It won't break if there are duplicate addresses, and it seems an existing behaviour to allow that happen.
3 participants
Change the per-address based outlier detection to per-endpoint based.
We use two maps: address map and endpoint status map.
We use old pick first leaf LB now. Later when we switch to new pick first leaf policy, create subchannel args always have single address eag. The change should work with both cases.
cc. @larry-safran @ejona86