ZOOKEEPER-2184 Zookeeper Client should re-resolve hosts when connection attempts fail #534

anmolnar · 2018-05-29T15:21:37Z

This is the master/3.5 port of #451

anmolnar · 2018-05-31T15:38:20Z

@hanm Did you have a chance to take a look?

hanm

Had a brief look, I think this pull request is missing some of the comment changes from #451:

The comments change in HostProvider interface regarding caching at different levels, etc.
Some comments in StaticHostProvider regarding the semantics of next().

lvfangmin · 2018-06-04T05:00:23Z

src/java/main/org/apache/zookeeper/client/StaticHostProvider.java

@@ -73,15 +80,27 @@
     *             if serverAddresses is empty or resolves to an empty list
     */
    public StaticHostProvider(Collection<InetSocketAddress> serverAddresses) {
-       sourceOfRandomness = new Random(System.currentTimeMillis() ^ this.hashCode());
+        init(serverAddresses,


Nit: Call StaticHostProvider(Collection serverAddresses, Resolver resolver).

Please change the other two constructor as well.

Sorry @lvfangmin , I might be missing the point here. Shall I change the signature to use non-generic Collection?

I think what @lvfangmin meant is instead of duplicating the code inside three constructors, only keep one concrete parameterized constructor implementation and let the other two invoke that one (with different parameters.).

What @hanm said, instead of call init, call this.

Tried that. Cannot call the other constructor, because this is referenced.

Yeah, I think referencing this before the object is constructed is prohibited. I think we can probably get around it but it looks like not worth the extra effort (I don't know a simple of way of doing it without dropping using the this.hashCode).
So I am ok with the current form of having all ctors invoking init method.

lvfangmin · 2018-06-04T05:39:54Z

src/java/main/org/apache/zookeeper/client/StaticHostProvider.java

@@ -314,7 +340,7 @@ public InetSocketAddress next(long spinDelay) {
                addr = nextHostInReconfigMode();
                if (addr != null) {
                	currentIndex = serverAddresses.indexOf(addr);
-                	return addr;                
+                	return resolve(addr);


Can you do some test to find out performance/latency of resolving an addr? If it's costly, maybe we should cache the resolved one instead of resolve it every time.

I disagree. We should not do any caching in our codebase, because there're multiple levels of caching already present in DNS infrastructure, like JVM caching, os-level caching, DNS servers caching, etc. resolve() will eventually become a no-op if any of these caches find a hit.

I know there is DNS caching, not aware that we have JVM or os caching for this, if we tested this is trivial, I totally agree we should keep this simple.

IMHO JVM caching is overkill too. It's not application's responsibility to deal with DNS re-resolution, because it shouldn't happen in failure scenarios only. On the flipside to avoid DNS server flooding some caching in the app would be reasonable and JVM is a standard way of doing that.

lvfangmin · 2018-06-04T05:41:25Z

src/java/main/org/apache/zookeeper/client/StaticHostProvider.java

            }
+            Collections.shuffle(resolvedAddresses);
+            return new InetSocketAddress(resolvedAddresses.get(0), address.getPort());
+        } catch (UnknownHostException e) {


Should log the exception here. Does the caller handle the unknown address correctly?

That's correct. The caller will end up getting UnknownHostException when trying to open the socket to the unresolvable address:

2018-06-04 12:31:26,022 [myid:huhuuhujkdshgfjksgd.com:2181] - WARN [main-SendThread(huhuuhujkdshgfjksgd.com:2181):ClientCnxn$SendThread@1237] - Session 0x0 for server huhuuhujkdshgfjksgd.com:2181, unexpected error, closing socket connection and attempting reconnect java.nio.channels.UnresolvedAddressException at sun.nio.ch.Net.checkAddress(Net.java:101) at sun.nio.ch.SocketChannelImpl.connect(SocketChannelImpl.java:622) at org.apache.zookeeper.ClientCnxnSocketNIO.registerAndConnect(ClientCnxnSocketNIO.java:275) at org.apache.zookeeper.ClientCnxnSocketNIO.connect(ClientCnxnSocketNIO.java:285) at org.apache.zookeeper.ClientCnxn$SendThread.startConnect(ClientCnxn.java:1091) at org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1133)

The reason for doing this way is to avoid API change of next().

Logging makes sense, I added an error log entry to make it clear.

anmolnar · 2018-06-04T10:23:45Z

@hanm Thanks, I updated the comments to be consistent with the original PR.
Unfortunately the comment of HostProvider wasn't updated, so I also made changes in there.

nkalmar

LGTM

hanm · 2018-06-08T21:25:15Z

src/java/main/org/apache/zookeeper/client/StaticHostProvider.java

@@ -73,15 +80,27 @@
     *             if serverAddresses is empty or resolves to an empty list
     */
    public StaticHostProvider(Collection<InetSocketAddress> serverAddresses) {
-       sourceOfRandomness = new Random(System.currentTimeMillis() ^ this.hashCode());
+        init(serverAddresses,


I think what @lvfangmin meant is instead of duplicating the code inside three constructors, only keep one concrete parameterized constructor implementation and let the other two invoke that one (with different parameters.).

hanm · 2018-06-08T21:33:00Z

src/java/main/org/apache/zookeeper/client/StaticHostProvider.java

-        // Resolve server addresses and shuffle them
-        List<InetSocketAddress> resolvedList = resolveAndShuffle(serverAddresses);
-        if (resolvedList.isEmpty()) {
+        List<InetSocketAddress> shuffledList = shuffle(serverAddresses);


I am not sure why we change the resolveAndShuffle to shuffle here, they are semantically different (one tries to resolve address, the other does not and only does shuffle.). The serverAddresses passed in this method is unresolved address, and we need it resolved because we rely on the resolved addresses to compare the old / new server list (in the context of probability rebalancing clients for dynamic reconfiguration).

Without resolving I think the client rebalance logic will be broken. A side note that all tests still passed probably indicate we don't have a 100% coverage for the logic in our tests.

After this change, the old / new server list both contains unresolved address, so it's comparable, but it's meaningless to compare addr.getAddress because it will return null when it's unresolved.

resolveAndShuffle has been split into two methods: resolve and shuffle, because we have to do it separately. Github shows the diff as original has been renamed and new one has been created.

I'll double check the rebalance functionality to make sure it's working properly and will also check related unit tests. I think what @lvfangmin is saying makes sense.

Comparison works for unresolved addresses too, because of the last if condition here:

if (addr.getPort() == myServer.getPort() && ((addr.getAddress() != null && myServer.getAddress() != null && addr .getAddress().equals(myServer.getAddress())) || addr .getHostString().equals(myServer.getHostString()))) {

As long as getHostString() works for unresolved addresses, we're fine. However, exactly the same functionality has already been implemented in the InetSocketAddress.equals() method, so I refactored this part to use it.

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.).

@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).

New impl

currentHost resolved, replaceHost unresolved => client should disconnect,

currentHost resolved, replaceHost resolved => client should not disconnect,

currentHost unresolved, replaceHost unresolved => client should disconnect,

currentHost unresolved, replaceHost resolved => client should disconnect

Old impl

currentHost resolved, replaceHost unresolved => client should not disconnect,

currentHost resolved, replaceHost resolved => client should not disconnect,

currentHost unresolved, replaceHost unresolved => client should disconnect,

currentHost unresolved, replaceHost resolved => client should not disconnect

Basically the difference is in the case when comparing resolved address with unresolved. The built-in implementation treats them as different address, hence forcing the client to disconnect which makes slightly more sense to me.

Anyway I'm happy to revert the change if you think this is not acceptable.

Thanks for quick update, @anmolnar !

The built-in implementation treats them as different address

That's what I am not sure about. If an unresolved address and a resolved address actually maps same address (after unresolved address gets resolved), should both be treated as same address (old behavior, via getHostString), instead of different address?

What I am thinking is to instead of using equal - use the old verbose code which will work (so far) for all combinations of comparing resolved and unresolved address. What do you think about this?

My only concern is with the first case: IP address might have changed of the host, so client should disconnect and trigger a re-resolution.

Anyway. No issue has been reported with the logic we're talking about, so let's just leave it out from this patch. Reverted.

hanm · 2018-06-18T03:37:58Z

@anmolnar do you have a chance to address the issues from code review? Please let me know if you plan to work on this in next few days - otherwise I'll just go ahead and do the 3.4 release without waiting on this merged to 3.5 and master.

anmolnar · 2018-06-18T04:44:57Z

@hanm Sorry for the delay. I'll work on it today.

…nit tests to cover unresolved hostnames cases

anmolnar · 2018-06-18T12:00:20Z

@hanm @lvfangmin Reconfig tests have previously been implemented only for IP addresses and hasn't covered hostnames which need to be resolved. I added a few unit tests for coverage, but found that existing tests should also be good to refactor.

I don't want to overload this patch, so will create a separate Jira for the refactoring instead.

anmolnar · 2018-06-18T12:53:15Z

Unit test refactoring: https://issues.apache.org/jira/browse/ZOOKEEPER-3065

anmolnar · 2018-07-09T20:24:05Z

@hanm @lvfangmin Are you happy with commiting the patch?

…esolved/unresolved addresses

anmolnar · 2018-07-11T20:47:26Z

@lvfangmin Unit tests failed, but it seems like we also introduced a findbug issue with #562
Shall I fix it in this patch or would you like to do it separately?

hanm · 2018-07-12T23:44:01Z

I think this one is good to commit - I'll commit in next few days if no one raise other concerns.

hanm · 2018-07-13T03:58:13Z

it seems like we also introduced a findbug issue with #562

Let's do this as a separate issue. I committed the patch, so I feel obligated to fix it:
#568

…ion attempts fail This is the master/3.5 port of #451 Author: Andor Molnar <andor@cloudera.com> Author: Andor Molnar <andor@apache.org> Reviewers: Michael Han <hanm@apache.org>, Flavio Junqueira <fpj@apache.org>, Edward Ribeiro <edward.ribeiro@gmail.com>, Mark Fenes <mfenes@cloudera.com>, Abraham Fine <afine@apache.org> Closes #534 from anmolnar/ZOOKEEPER-2184_master (cherry picked from commit 0a31187) Signed-off-by: Michael Han <hanm@apache.org>

…ion attempts fail This is the master/3.5 port of apache#451 Author: Andor Molnar <andor@cloudera.com> Author: Andor Molnar <andor@apache.org> Reviewers: Michael Han <hanm@apache.org>, Flavio Junqueira <fpj@apache.org>, Edward Ribeiro <edward.ribeiro@gmail.com>, Mark Fenes <mfenes@cloudera.com>, Abraham Fine <afine@apache.org> Closes apache#534 from anmolnar/ZOOKEEPER-2184_master

anmolnar added 2 commits May 29, 2018 17:19

ZOOKEEPER-2184. Ported the patch from branch-3.4

98ea004

ZOOKEEPER-2184. Accidently removed index initializations

5660d6b

hanm requested changes Jun 1, 2018

View reviewed changes

lvfangmin reviewed Jun 4, 2018

View reviewed changes

ZOOKEEPER-2184. Added comments to be consistent with apache#451

41b01c8

ZOOKEEPER-2184. Added error log if address is not resolvable

d2a5696

anmolnar force-pushed the ZOOKEEPER-2184_master branch from 62a8978 to d2a5696 Compare June 4, 2018 11:43

nkalmar approved these changes Jun 5, 2018

View reviewed changes

hanm requested changes Jun 8, 2018

View reviewed changes

ZOOKEEPER-2184. Use InetSocketAddress.equals() for eq check + added u…

4310b6c

…nit tests to cover unresolved hostnames cases

anmolnar force-pushed the ZOOKEEPER-2184_master branch 2 times, most recently from a2565d7 to b557298 Compare July 11, 2018 15:30

ZOOKEEPER-2184: Added 4 test cases to cover updateServerList() with r…

420f050

…esolved/unresolved addresses

anmolnar force-pushed the ZOOKEEPER-2184_master branch from b557298 to 420f050 Compare July 11, 2018 20:19

hanm approved these changes Jul 12, 2018

View reviewed changes

ZOOKEEPER-2184. Revert equality logic in updateServerList()

6bccacd

anmolnar force-pushed the ZOOKEEPER-2184_master branch from be04719 to 6bccacd Compare July 13, 2018 10:49

asfgit closed this in 0a31187 Jul 16, 2018

anmolnar deleted the ZOOKEEPER-2184_master branch July 16, 2018 09:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZOOKEEPER-2184 Zookeeper Client should re-resolve hosts when connection attempts fail #534

ZOOKEEPER-2184 Zookeeper Client should re-resolve hosts when connection attempts fail #534

anmolnar commented May 29, 2018

anmolnar commented May 31, 2018

hanm left a comment

lvfangmin Jun 4, 2018

anmolnar Jun 4, 2018

hanm Jun 8, 2018

lvfangmin Jun 9, 2018 •

edited

Loading

anmolnar Jun 18, 2018

hanm Jul 10, 2018

lvfangmin Jun 4, 2018

anmolnar Jun 4, 2018

lvfangmin Jun 9, 2018

anmolnar Jun 18, 2018 •

edited

Loading

lvfangmin Jun 4, 2018

anmolnar Jun 4, 2018 •

edited

Loading

anmolnar commented Jun 4, 2018

nkalmar left a comment

hanm Jun 8, 2018

hanm Jun 8, 2018

lvfangmin Jun 9, 2018

anmolnar Jun 18, 2018

anmolnar Jun 18, 2018

hanm Jul 10, 2018

anmolnar Jul 11, 2018 •

edited

Loading

hanm Jul 12, 2018

anmolnar Jul 12, 2018

hanm commented Jun 18, 2018

anmolnar commented Jun 18, 2018

anmolnar commented Jun 18, 2018 •

edited

Loading

anmolnar commented Jun 18, 2018

anmolnar commented Jul 9, 2018

anmolnar commented Jul 11, 2018

hanm commented Jul 12, 2018

hanm commented Jul 13, 2018

ZOOKEEPER-2184 Zookeeper Client should re-resolve hosts when connection attempts fail #534

ZOOKEEPER-2184 Zookeeper Client should re-resolve hosts when connection attempts fail #534

Conversation

anmolnar commented May 29, 2018

anmolnar commented May 31, 2018

hanm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lvfangmin Jun 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmolnar Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmolnar Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

anmolnar commented Jun 4, 2018

nkalmar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmolnar Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanm commented Jun 18, 2018

anmolnar commented Jun 18, 2018

anmolnar commented Jun 18, 2018 • edited Loading

anmolnar commented Jun 18, 2018

anmolnar commented Jul 9, 2018

anmolnar commented Jul 11, 2018

hanm commented Jul 12, 2018

hanm commented Jul 13, 2018

lvfangmin Jun 9, 2018 •

edited

Loading

anmolnar Jun 18, 2018 •

edited

Loading

anmolnar Jun 4, 2018 •

edited

Loading

anmolnar Jul 11, 2018 •

edited

Loading

anmolnar commented Jun 18, 2018 •

edited

Loading