Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/534#discussion_r201456281
--- 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 --
>> However, exactly the same functionality has already been implemented in
the InetSocketAddress.equals() method
I am wondering if this is the case or not. I just did a random peek at
https://github.com/openjdk-mirror/jdk7u-jdk/blob/master/src/share/classes/java/net/InetSocketAddress.java#L300,
looks like if we compare an unresolved address with a resolved address, the
equals will return false - but the code you pasted will return true if
getHostString works for both resolved and unresolved address... could you
double check this behavior?
Also, is it possible to add a test case to cover the case where the second
parameter of updateServerList is a resolved address? The existing test cases
only cover the case where the second parameter (myServer) is unresolved. In
practice I think the method updateServerList is called by ZooKeeper's
updateServerList method with second parameter as a resolved address (the remote
server where current client connected to.).
---
