ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-776

API should sanity check sessionTimeout argument

    Details

    • Type: Improvement Improvement
    • Status: Patch Available
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 3.2.2, 3.3.0, 3.3.1, 3.4.6, 3.5.0
    • Fix Version/s: 3.5.2, 3.6.0
    • Component/s: c client, java client
    • Labels:
      None
    • Environment:

      OSX 10.6.3, JVM 1.6.0-20

      Description

      passing in a "0" sessionTimeout to ZooKeeper() constructor leads to errors in subsequent operations. It would be ideal to capture this configuration error at the source by throwing something like an IllegalArgument exception when the bogus sessionTimeout is specified, instead of later when it is utilized.

      1. zookeeper-776-fix.patch
        1 kB
        Gregory Haskins
      2. zookeeper-776-fix.patch
        3 kB
        Gregory Haskins
      3. zookeeper-776-fix.patch
        2 kB
        Gregory Haskins
      4. ZOOKEEPER-776.patch
        6 kB
        Raul Gutierrez Segales

        Issue Links

          Activity

          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Patch Available Patch Available
          6h 27m 3 Gregory Haskins 21/May/10 22:07
          Patch Available Patch Available Open Open
          5d 19h 45m 4 Bill Havanki 18/Jul/14 14:24
          Open Open In Progress In Progress
          1512d 20h 59m 2 Bill Havanki 18/Jul/14 14:24
          In Progress In Progress Patch Available Patch Available
          335d 4h 2 Raul Gutierrez Segales 18/Jun/15 18:25
          Hide
          Bill Havanki added a comment -

          No worries! I've been working on other things, so I haven't been blocked at all. Thanks for taking this issue back up!

          Show
          Bill Havanki added a comment - No worries! I've been working on other things, so I haven't been blocked at all. Thanks for taking this issue back up!
          Raul Gutierrez Segales made changes -
          Status In Progress [ 3 ] Patch Available [ 10002 ]
          Affects Version/s 3.5.0 [ 12316644 ]
          Affects Version/s 3.4.6 [ 12323310 ]
          Hide
          Raul Gutierrez Segales added a comment -

          (assigning to me so i can submit the patch, but this is Bill's patch)

          Show
          Raul Gutierrez Segales added a comment - (assigning to me so i can submit the patch, but this is Bill's patch)
          Raul Gutierrez Segales made changes -
          Assignee Bill Havanki [ bhavanki ] Raul Gutierrez Segales [ rgs ]
          Raul Gutierrez Segales made changes -
          Assignee Bill Havanki [ bhavanki ]
          Raul Gutierrez Segales made changes -
          Attachment ZOOKEEPER-776.patch [ 12740443 ]
          Hide
          Raul Gutierrez Segales added a comment -

          uploading Bill Havanki's patch from reviewboard

          (sorry for dropping the ball for a year here)

          Show
          Raul Gutierrez Segales added a comment - uploading Bill Havanki 's patch from reviewboard (sorry for dropping the ball for a year here)
          Hide
          Raul Gutierrez Segales added a comment -

          sorry about the delay Bill Havanki, some how i lost track of this. reviewing now.

          Show
          Raul Gutierrez Segales added a comment - sorry about the delay Bill Havanki , some how i lost track of this. reviewing now.
          Bill Havanki made changes -
          Assignee Bill Havanki [ bhavanki ]
          Michi Mutsuzaki made changes -
          Fix Version/s 3.5.2 [ 12331981 ]
          Fix Version/s 3.6.0 [ 12326518 ]
          Fix Version/s 3.5.1 [ 12326786 ]
          Patrick Hunt made changes -
          Fix Version/s 3.5.1 [ 12326786 ]
          Fix Version/s 3.5.0 [ 12316644 ]
          Bill Havanki made changes -
          Status Open [ 1 ] In Progress [ 3 ]
          Bill Havanki made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Bill Havanki made changes -
          Remote Link This issue links to "Review (Web Link)" [ 15914 ]
          Bill Havanki made changes -
          Status In Progress [ 3 ] Patch Available [ 10002 ]
          Release Note The attached patch is a proposed fix for the java client
          Hide
          Bill Havanki added a comment -
          Show
          Bill Havanki added a comment - Review available: https://reviews.apache.org/r/23678/
          Bill Havanki made changes -
          Status Open [ 1 ] In Progress [ 3 ]
          Bill Havanki made changes -
          Assignee Bill Havanki [ bhavanki ]
          Hide
          Gregory Haskins added a comment -

          Go for it Bill! I obviously became distracted

          Show
          Gregory Haskins added a comment - Go for it Bill! I obviously became distracted
          Hide
          Raul Gutierrez Segales added a comment -

          happy to help with reviews, Bill Havanki.

          Show
          Raul Gutierrez Segales added a comment - happy to help with reviews, Bill Havanki .
          Hide
          Bill Havanki added a comment -

          Anyone mind if I grab this ticket? It's been idle for over 4 years, but it's targeted for 3.5.0. I'll assign myself soon unless I hear any objections.

          Fair warning: I haven't done C++ coding since the last millennium.

          Show
          Bill Havanki added a comment - Anyone mind if I grab this ticket? It's been idle for over 4 years, but it's targeted for 3.5.0. I'll assign myself soon unless I hear any objections. Fair warning: I haven't done C++ coding since the last millennium.
          Mahadev konar made changes -
          Fix Version/s 3.5.0 [ 12316644 ]
          Fix Version/s 3.4.0 [ 12314469 ]
          Hide
          Gregory Haskins added a comment -

          Heh, noted.

          I just submitted a new patch that uses System.getProperties() instead.

          I havent tried setting up a C development environment for zookeeper yet, so I will have to look at that next.

          Show
          Gregory Haskins added a comment - Heh, noted. I just submitted a new patch that uses System.getProperties() instead. I havent tried setting up a C development environment for zookeeper yet, so I will have to look at that next.
          Gregory Haskins made changes -
          Attachment zookeeper-776-fix.patch [ 12445784 ]
          Hide
          Benjamin Reed added a comment -

          you were a little over ambitious gregory you don't need to load from a properties file. just check the system property and if it isn't sent (or not a number) use 100.

          are you going to be able to patch the C client too?

          Show
          Benjamin Reed added a comment - you were a little over ambitious gregory you don't need to load from a properties file. just check the system property and if it isn't sent (or not a number) use 100. are you going to be able to patch the C client too?
          Gregory Haskins made changes -
          Attachment zookeeper-776-fix.patch [ 12445697 ]
          Hide
          Gregory Haskins added a comment -

          This is an updated version that supports a default minTimeout of 100, and loading a new default from zookeeper.properties -> zookeeper.minTimeout

          Show
          Gregory Haskins added a comment - This is an updated version that supports a default minTimeout of 100, and loading a new default from zookeeper.properties -> zookeeper.minTimeout
          Hide
          Gregory Haskins added a comment -

          I will take a crack at it

          Show
          Gregory Haskins added a comment - I will take a crack at it
          Benjamin Reed made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Benjamin Reed added a comment -

          there are two things we need to fix:

          1) this just pushes the problems slightly. someone can still use a timeout of 1 and run into the same problem. we should make the minimum timeout configurable using a system property. (i would suggest "zookeeper.minTimeout") and set it to something reasonable like 100.
          2) we should also put a similar check in the C client.

          let me know if you are willing to make those changes. i'd be glad to pitch in if you cannot.

          Show
          Benjamin Reed added a comment - there are two things we need to fix: 1) this just pushes the problems slightly. someone can still use a timeout of 1 and run into the same problem. we should make the minimum timeout configurable using a system property. (i would suggest "zookeeper.minTimeout") and set it to something reasonable like 100. 2) we should also put a similar check in the C client. let me know if you are willing to make those changes. i'd be glad to pitch in if you cannot.
          Hide
          Henry Robinson added a comment -

          Greg -

          Don't worry - you should have seen the hash I made of my first patch!

          Hudson is misbehaving at the moment, so I'm not convinced that the test failures are as a result of your patch. You don't need to do anything right now - I'll take a look and update this ticket once I know what's going on.

          cheers,
          Henry

          Show
          Henry Robinson added a comment - Greg - Don't worry - you should have seen the hash I made of my first patch! Hudson is misbehaving at the moment, so I'm not convinced that the test failures are as a result of your patch. You don't need to do anything right now - I'll take a look and update this ticket once I know what's going on. cheers, Henry
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12445200/zookeeper-776-fix.patch
          against trunk revision 947063.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/102/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/102/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/102/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12445200/zookeeper-776-fix.patch against trunk revision 947063. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/102/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/102/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/102/console This message is automatically generated.
          Hide
          Gregory Haskins added a comment -

          Ah, sorry, I had already deleted it before I saw your latest comment. I seem to be making a general mess of this JIRA entry. Bear with me while I learn

          In any case, I have submitted a fix that has a JUnit and a --no-prefix export, so hopefully Hudson is happier this time. I confirmed that the test fails without the fix, and passes with the fix. Let me know if there is anything else I should be doing.

          Thanks for all the help,
          -Greg

          Show
          Gregory Haskins added a comment - Ah, sorry, I had already deleted it before I saw your latest comment. I seem to be making a general mess of this JIRA entry. Bear with me while I learn In any case, I have submitted a fix that has a JUnit and a --no-prefix export, so hopefully Hudson is happier this time. I confirmed that the test fails without the fix, and passes with the fix. Let me know if there is anything else I should be doing. Thanks for all the help, -Greg
          Hide
          Henry Robinson added a comment -

          Cancelling the patch is fine but there's no need to delete it - Hudson will always figure out what the latest patch is and it's good to see how a ticket evolved.

          Tests will also help

          Show
          Henry Robinson added a comment - Cancelling the patch is fine but there's no need to delete it - Hudson will always figure out what the latest patch is and it's good to see how a ticket evolved. Tests will also help
          Gregory Haskins made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Gregory Haskins made changes -
          Attachment zookeeper-776-fix.patch [ 12445200 ]
          Gregory Haskins made changes -
          Attachment zookeeper-776-fix.patch [ 12445195 ]
          Gregory Haskins made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Gregory Haskins added a comment -

          Hi Henry,

          I'll do one better and add a JUnit to address the first "-1" as well

          Im just testing the junit now, and will submit the updated patch shortly. Should I cancel the previous patch and delete the old attachment?

          Show
          Gregory Haskins added a comment - Hi Henry, I'll do one better and add a JUnit to address the first "-1" as well Im just testing the junit now, and will submit the updated patch shortly. Should I cancel the previous patch and delete the old attachment?
          Hide
          Henry Robinson added a comment -

          Thanks Greg - can you generate your patch from git with --no-prefix, to make it svn compatible?

          Show
          Henry Robinson added a comment - Thanks Greg - can you generate your patch from git with --no-prefix, to make it svn compatible?
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12445195/zookeeper-776-fix.patch
          against trunk revision 947063.

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no tests are needed for this patch.

          -1 patch. The patch command could not apply the patch.

          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/101/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12445195/zookeeper-776-fix.patch against trunk revision 947063. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. -1 patch. The patch command could not apply the patch. Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/101/console This message is automatically generated.
          Gregory Haskins made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Release Note The attached patch is a proposed fix for the java client
          Gregory Haskins made changes -
          Attachment zookeeper-776-fix.patch [ 12445195 ]
          Gregory Haskins made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Gregory Haskins made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Gregory Haskins added a comment -

          commit 840f56d388582e1df39f7513aa7f4d4ce0610718
          Author: Gregory Haskins <ghaskins@novell.com>
          Date: Fri May 21 14:58:14 2010 -0400

          javaclient: validate sessionTimeout field at ZooKeeper init

          JIRA ZOOKEEPER-776 describes the following problem:

          passing in a "0" sessionTimeout to ZooKeeper() constructor leads to errors
          in subsequent operations. It would be ideal to capture this configuration
          error at the source by throwing something like an IllegalArgument exception
          when the bogus sessionTimeout is specified, instead of later when it is
          utilized.

          This patch is a proposal to fix the problem referenced above.

          Signed-off-by: Gregory Haskins <ghaskins@novell.com>

          diff --git a/src/java/main/org/apache/zookeeper/ClientCnxn.java b/src/java/main/
          index 8eb227d..682811b 100644
          — a/src/java/main/org/apache/zookeeper/ClientCnxn.java
          +++ b/src/java/main/org/apache/zookeeper/ClientCnxn.java
          @@ -353,6 +353,11 @@ public class ClientCnxn {
          this.sessionId = sessionId;
          this.sessionPasswd = sessionPasswd;

          + if (sessionTimeout <= 0)

          { + throw new IOException("sessionTimeout " + sessionTimeout + + " is not valid"); + }

          +
          // parse out chroot, if any
          int off = hosts.indexOf('/');
          if (off >= 0) {

          Show
          Gregory Haskins added a comment - commit 840f56d388582e1df39f7513aa7f4d4ce0610718 Author: Gregory Haskins <ghaskins@novell.com> Date: Fri May 21 14:58:14 2010 -0400 javaclient: validate sessionTimeout field at ZooKeeper init JIRA ZOOKEEPER-776 describes the following problem: passing in a "0" sessionTimeout to ZooKeeper() constructor leads to errors in subsequent operations. It would be ideal to capture this configuration error at the source by throwing something like an IllegalArgument exception when the bogus sessionTimeout is specified, instead of later when it is utilized. This patch is a proposal to fix the problem referenced above. Signed-off-by: Gregory Haskins <ghaskins@novell.com> diff --git a/src/java/main/org/apache/zookeeper/ClientCnxn.java b/src/java/main/ index 8eb227d..682811b 100644 — a/src/java/main/org/apache/zookeeper/ClientCnxn.java +++ b/src/java/main/org/apache/zookeeper/ClientCnxn.java @@ -353,6 +353,11 @@ public class ClientCnxn { this.sessionId = sessionId; this.sessionPasswd = sessionPasswd; + if (sessionTimeout <= 0) { + throw new IOException("sessionTimeout " + sessionTimeout + + " is not valid"); + } + // parse out chroot, if any int off = hosts.indexOf('/'); if (off >= 0) {
          Patrick Hunt made changes -
          Field Original Value New Value
          Fix Version/s 3.4.0 [ 12314469 ]
          Component/s c client [ 12312380 ]
          Hide
          Patrick Hunt added a comment -

          This would be a good hole to close. We should throw the exception/error & log when 0 is passed. We need to fix java and also verify/fix C client code. This would be an excellent first project for someone interested in becoming a contributor to zk.

          Show
          Patrick Hunt added a comment - This would be a good hole to close. We should throw the exception/error & log when 0 is passed. We need to fix java and also verify/fix C client code. This would be an excellent first project for someone interested in becoming a contributor to zk.
          Gregory Haskins created issue -

            People

            • Assignee:
              Raul Gutierrez Segales
              Reporter:
              Gregory Haskins
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:

                Development