ZOOKEEPER-2251:Add Client side packet response timeout to avoid infinite wait. #119

arshadmohammad · 2016-12-05T18:58:39Z

Add Client side packet response timeout to avoid infinite wait.

hanm

Had a quick look over the patch and here is initial feedback.

hanm · 2016-12-09T06:52:23Z

src/java/main/org/apache/zookeeper/ClientCnxn.java

            while (!packet.finished) {
-                packet.wait();
+                packet.wait(requestTimeout);


This unconditionally adds a timeout is a pretty significant semantic change. Whether or not we should wait indefinitely is a separate issue to discuss later, but from a compatibility point of view we'd at least provide a configuration option to let user opt in this feature rather than having this turned on by default. So by default there is no timeout and client still waits indefinitely, user who wants to opt in to turn on the timeout needs to explicit say so and provide a value they think make sense to them. Otherwise, we'd have to solve the problem of what the default timeout value we should set in ZK config if this feature is turned on by default, and that problem is hard and very likely there is no default value that makes everyone happy.

disabled this feature by default. any non positive value will disable this feature

hanm · 2016-12-09T06:56:45Z

src/java/main/org/apache/zookeeper/KeeperException.java

@@ -387,6 +389,8 @@ public void setCode(int code) {
        EPHEMERALONLOCALSESSION (EphemeralOnLocalSession),
        /** Attempts to remove a non-existing watcher */
        NOWATCHER (-121),
+        /** Not received packet timely */
+        TIMEOUT (-122),


Similar error code needs to be added to C client to make both client library compatible.

I think adding only error code will not be useful. Do you mean, add response timeout functionality in C code?

I was thinking the same error code should be added to C client so the C client can react to the new error code properly, instead of crash (the worst case). Though, I suspect that the C client might be just fine as this error code will not be fired on server side (it's a pure Java client thing), so maybe we are fine without touch C client.

We already have OPERATIONTIMEOUT error code, so TIMEOUT sound too generic. Instead of using TIMEOUT, would something more descriptive better? I am thinking SERVERRESPONSETIMEOUT (server response timeout). Consider this is user facing, have a descriptive name with clear semantic sounds better.

Good catch. Request time out is also a kind of operation time out. OPERATIONTIMEOUT, I think it is not used any where in the java client code or server code, not sure about other client codes. leaving OPERATIONTIMEOUT as it is.
Changing TIMEOUT to SERVERRESPONSETIMEOUT is OK but I do not want to change property name. Can we use REQUESTTIMEOUT.

REQUESTTIMEOUT sounds good to me.

hanm · 2016-12-09T06:57:31Z

src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml

+                <term>zookeeper.request.timeout</term>
+                <listitem>
+                <para>
+                    <emphasis role="bold">New in 3.5.3:</emphasis>


This probably would not make to 3.5.3. 3.6.0 would be a more realistic value.

wuwen5 · 2016-12-09T09:28:40Z

src/java/main/org/apache/zookeeper/ClientCnxn.java

            while (!packet.finished) {
-                packet.wait();
+                packet.wait(requestTimeout);
+                if (!packet.finished && ((System.currentTimeMillis()


System.currentTimeMillis() is dependent on System clock. It probably micro-corrected by an external programme..

I think using System.nanoTime() to measure elapsed time is the correct solution.

Yes, and actually we have a Time class which we can use here that uses nanoTime internally for exact same reason.

anmolnar · 2018-07-17T10:41:01Z

@hanm @arshadmohammad This PR hasn't been updated for ages unfortunately, but looks like it indicates a pretty significant bug in Zk client library. Additionally we could also fix an long outstanding flaky test by fixing this, so killing two birds with a single shot.

Although Akihiro's comment on the jira sounds reasonable: by using TCP there's no such thing as "packet loss" and we should better try to find the bug in Zookeeper code, I believe that waiting indefinitely is generally a bad thing and introducing a reasonable and configurable timeout would be beneficial.

Introducing such thing in 3.6 is no harm and I would consider it merging to 3.5 too.

hanm · 2018-07-17T19:02:27Z

Sounds good to me. I had some comments before and I believe those are still applicable.

arshadmohammad · 2018-07-18T02:37:02Z

Thanks @anmolnar @hanm for your reviews, I will address the comments and update MR.

hanm

I have two minor comments regarding naming of the new keeper exception error code. Rest of the patch looks great.

hanm · 2018-07-24T05:20:51Z

src/java/main/org/apache/zookeeper/KeeperException.java

@@ -387,6 +389,8 @@ public void setCode(int code) {
        EPHEMERALONLOCALSESSION (EphemeralOnLocalSession),
        /** Attempts to remove a non-existing watcher */
        NOWATCHER (-121),
+        /** Not received packet timely */
+        TIMEOUT (-122),


We already have OPERATIONTIMEOUT error code, so TIMEOUT sound too generic. Instead of using TIMEOUT, would something more descriptive better? I am thinking SERVERRESPONSETIMEOUT (server response timeout). Consider this is user facing, have a descriptive name with clear semantic sounds better.

hanm · 2018-07-24T05:21:17Z

src/java/main/org/apache/zookeeper/KeeperException.java

+    /**
+     * @see Code#TIMEOUT
+     */
+    public static class TimeoutException extends KeeperException {


Similarly, use a more descriptive name like ServerResponseTimeoutException sounds better to me.

Changed to RequestTimeoutException

eolivelli · 2018-07-24T05:38:46Z

src/java/test/org/apache/zookeeper/ClientResponseTimeoutTest.java

+import org.junit.Assert;
+import org.junit.Test;
+
+public class ClientResponseTimeoutTest extends QuorumPeerTestBase {


Not sure we need to start a full cluster, what about adding a bunch of mockito based tests?

I think that having a test which uses the full stack may have some value, so maybe we can keep this. But in case of failure we are not sure about the real cause

eolivelli · 2018-07-24T05:41:08Z

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

@@ -56,9 +56,15 @@
    @SuppressWarnings("deprecation")
    public static final String SECURE_CLIENT = ZooKeeper.SECURE_CLIENT;
    public static final int CLIENT_MAX_PACKET_LENGTH_DEFAULT = 4096 * 1024; /* 4 MB */
+    public static final String ZOOKEEPER_REQUEST_TIMEOUT = "zookeeper.request.timeout";
+    /**
+     * Feature is disabled by default.


What about enabling it by default with a default computed from desired session expire timeout?

Session timeout and this client request timeout are completely different. I think we should not mix these two.

Totally agree, they are very different things. I mentioned it because configured session timeout is like a client side hint about how much time you can tolerate to be disconnected/not responsive, but it is better to keep it separate from this low level knob.
Anyway I wonder if it would be better to set a default timeout.
If there is a chance of infinite wait maybe it can be worth.
We can use a very large timeout, in the order of minutes.
Tipical session timeout is in the order of seconds

I tend to learn towards the conservative side to have this feature turned off by default, because I feel we don't have deep insight regarding what's the value we could use as default value (if we turn the feature on by default), and a premature setting of that value might impact existing users. For few of users who do have the problem they can tune the parameter based on their prod env.

Okay, no default

hanm · 2018-07-26T04:24:05Z

This is good to go - I am kicking the bot and once it's green I'll commit this.

…nite wait. Add Client side packet response timeout to avoid infinite wait. Author: Mohammad Arshad <arshad@apache.org> Reviewers: Michael Han <hanm@apache.org>, nrico Olivelli <eolivelli@gmail.com> Closes #119 from arshadmohammad/ZOOKEEPER-2251 (cherry picked from commit 9f82798) Signed-off-by: Michael Han <hanm@apache.org>

hanm · 2018-07-27T03:17:03Z

committed to 3.6/3.5. thanks @arshadmohammad !

…nite wait. Add Client side packet response timeout to avoid infinite wait. Author: Mohammad Arshad <arshad@apache.org> Reviewers: Michael Han <hanm@apache.org>, nrico Olivelli <eolivelli@gmail.com> Closes apache#119 from arshadmohammad/ZOOKEEPER-2251

hanm reviewed Dec 9, 2016

View reviewed changes

wuwen5 reviewed Dec 9, 2016

View reviewed changes

hanm reviewed Jul 24, 2018

View reviewed changes

eolivelli reviewed Jul 24, 2018

View reviewed changes

ZOOKEEPER-2251:Add Client side request time-out to avoid infinite wait

83946d6

asfgit closed this in 9f82798 Jul 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZOOKEEPER-2251:Add Client side packet response timeout to avoid infinite wait. #119

ZOOKEEPER-2251:Add Client side packet response timeout to avoid infinite wait. #119

arshadmohammad commented Dec 5, 2016

hanm left a comment

hanm Dec 9, 2016

arshadmohammad Jul 20, 2018

hanm Dec 9, 2016

arshadmohammad Jul 20, 2018

hanm Jul 20, 2018 •

edited

hanm Jul 24, 2018

arshadmohammad Jul 24, 2018

hanm Jul 25, 2018

hanm Dec 9, 2016

wuwen5 Dec 9, 2016

hanm Dec 15, 2016

arshadmohammad Jul 20, 2018

anmolnar commented Jul 17, 2018

hanm commented Jul 17, 2018

arshadmohammad commented Jul 18, 2018

hanm left a comment

hanm Jul 24, 2018

hanm Jul 24, 2018

arshadmohammad Jul 24, 2018

eolivelli Jul 24, 2018

eolivelli Jul 24, 2018

arshadmohammad Jul 24, 2018

eolivelli Jul 24, 2018

hanm Jul 25, 2018

eolivelli Jul 25, 2018 •

edited

hanm commented Jul 26, 2018

hanm commented Jul 27, 2018

ZOOKEEPER-2251:Add Client side packet response timeout to avoid infinite wait. #119

ZOOKEEPER-2251:Add Client side packet response timeout to avoid infinite wait. #119

Conversation

arshadmohammad commented Dec 5, 2016

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

Choose a reason for hiding this comment

hanm Jul 20, 2018 • edited

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

Choose a reason for hiding this comment

anmolnar commented Jul 17, 2018

hanm commented Jul 17, 2018

arshadmohammad commented Jul 18, 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

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

eolivelli Jul 25, 2018 • edited

Choose a reason for hiding this comment

hanm commented Jul 26, 2018

hanm commented Jul 27, 2018

hanm Jul 20, 2018 •

edited

eolivelli Jul 25, 2018 •

edited