RoundRobinLoadBalancer should not sort the addresses

[NiteshKant]

Motivation

For quicker lookups on receiving Service discovery events, RoundRobinLoadBalancer keeps a sorted list of addresses. This results in the same order of hosts for all client instances hence potentially creating hot spots. As, the sorting is not required functionally, we should avoid doing that.

Modification

Use unsorted address list and let service discoverer maintain order of addresses.

Result

RoundRobinLoadBalancer does not store sorted addresses.

[idelpivnitskiy]

LGTM, thanks! One question:

[Scottmitch]

lgtm with suggestion. I can submit in followup PR for discussion if you prefer.

[Scottmitch]

It is expected that the ServiceDiscoverer will filter duplicate events, and there is no modification outside of Subscriber#onNext(other than emptying the list). I think we can assume we won't find any duplicates on addition:

This will also avoid ArrayList copy/move of array elements after it has already been copied in the constructor.

                            final ResolvedAddress addr = requireNonNull(event.address());
                            final List<Host<ResolvedAddress, C>> refreshedAddresses;
                            if (event.isAvailable()) {
                                refreshedAddresses = new ArrayList<>(currentAddresses.size() + 1);
                                refreshedAddresses.addAll(currentAddresses);
                                refreshedAddresses.add(new Host<>(addr));
                            } else if (currentAddresses.isEmpty()) {
                                refreshedAddresses = currentAddresses;
                            } else {
                                refreshedAddresses = new ArrayList<>(currentAddresses.size() - 1);
                                for (Host<ResolvedAddress, C> host : (List<Host<ResolvedAddress, C>>) currentAddresses) {
                                    if (host.address.equals(addr)) {
                                        host.markInactive();
                                    } else {
                                        refreshedAddresses.add(host);
                                    }
                                }
                            }
                            return refreshedAddresses;

[idelpivnitskiy]

I was thinking about double copying when we add an element. Unfortunately, refreshedAddresses.addAll(currentAddresses) does 2 copies internally. So, it will be the same as using new ArrayList(anotherList) + add with resize.

It is expected that the ServiceDiscoverer will filter duplicate events

DNS SD currently does not filter duplicates. It takes a List from netty's resolver.resolveAll and passes it as-is. I didn't check netty's code but assume it just parses each DnsRecord from the response. If DNS response contains duplicated addresses, we will see them here.

[NiteshKant]

I think we can assume we won't find any duplicates on addition

Actually I think we should allow duplicates which enables folks to effectively add weights to certain hosts. I would prefer not to do these two changes (unsorted and allow dups) in the same PR though. So, ya feel free to send a followup

[Scottmitch]

#1052