Github user anmolnar commented on a diff in the pull request:
    --- Diff: src/java/main/org/apache/zookeeper/client/ 
    @@ -149,15 +185,12 @@ public 
StaticHostProvider(Collection<InetSocketAddress> serverAddresses,
          * @param currentHost the host to which this client is currently 
          * @return true if changing connections is necessary for 
load-balancing, false otherwise  
         public synchronized boolean updateServerList(
                 Collection<InetSocketAddress> serverAddresses,
                 InetSocketAddress currentHost) {
    -        // Resolve server addresses and shuffle them
    -        List<InetSocketAddress> resolvedList = 
    -        if (resolvedList.isEmpty()) {
    +        List<InetSocketAddress> shuffledList = shuffle(serverAddresses);
    --- End diff --
    @hanm Good catch, that makes perfect sense. The implementation is slightly 
    I added 4 new test cases to cover the different combinations of the 2nd and 
3rd parameter. Some of them could be redundant (as you said tests present 
already), but I've already opened a Jira to clean-up this test file, so it 
should be okay. Test scenarios:
    *Given:* list of servers contains 1 element, client is connected 
(currentHost (myServer) != null), trying to replace server list with the same 
address (replaceHost).
    1. `currentHost` resolved, `replaceHost` unresolved => client should 
    2. `currentHost` resolved, `replaceHost` resolved => client should *not* 
    3. `currentHost` unresolved, `replaceHost` unresolved => client should 
    4. `currentHost` unresolved, `replaceHost` resolved => client should 
    My implementation covers all of these scenarios. The only difference is in 
*case no.4* where the original impl returns false (client should not 
disconnect). Question is what we would like to do in the 4th case? Does it make 
sense at all, as you mentioned client should not be connected to an unresolved 


Reply via email to