Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/534#discussion_r194186665
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -149,15 +185,12 @@ public
StaticHostProvider(Collection<InetSocketAddress> serverAddresses,
* @param currentHost the host to which this client is currently
connected
* @return true if changing connections is necessary for
load-balancing, false otherwise
*/
-
-
@Override
public synchronized boolean updateServerList(
Collection<InetSocketAddress> serverAddresses,
InetSocketAddress currentHost) {
- // Resolve server addresses and shuffle them
- List<InetSocketAddress> resolvedList =
resolveAndShuffle(serverAddresses);
- if (resolvedList.isEmpty()) {
+ List<InetSocketAddress> shuffledList = shuffle(serverAddresses);
--- End diff --
I am not sure why we change the `resolveAndShuffle` to `shuffle` here, they
are semantically different (one tries to resolve address, the other does not
and only does shuffle.). The `serverAddresses` passed in this method is
unresolved address, and we need it resolved because we rely on the resolved
addresses to compare the old / new server list (in the context of probability
rebalancing clients for dynamic reconfiguration).
Without resolving I think the client rebalance logic will be broken. A side
note that all tests still passed probably indicate we don't have a 100%
coverage for the logic in our tests.
---
