Github user anmolnar commented on a diff in the pull request:

    https://github.com/apache/zookeeper/pull/534#discussion_r201723327
  
    --- 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 --
    
    @hanm Good catch, that makes perfect sense. The implementation is slightly 
different.
    
    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 
disconnect,
    2. `currentHost` resolved, `replaceHost` resolved => client should *not* 
disconnect,
    3. `currentHost` unresolved, `replaceHost` unresolved => client should 
disconnect,
    4. `currentHost` unresolved, `replaceHost` resolved => client should 
disconnect
    
    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 
address?


---

Reply via email to