zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From anmolnar <...@git.apache.org>
Subject [GitHub] zookeeper pull request #534: ZOOKEEPER-2184 Zookeeper Client should re-resol...
Date Wed, 11 Jul 2018 14:57:18 GMT
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?


---

Mime
View raw message