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?
---
