ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-776

API should sanity check sessionTimeout argument

    Details

    • Type: Improvement Improvement
    • Status: In Progress
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 3.2.2, 3.3.0, 3.3.1
    • Fix Version/s: 3.5.1
    • 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
        2 kB
        Gregory Haskins
      2. zookeeper-776-fix.patch
        3 kB
        Gregory Haskins
      3. zookeeper-776-fix.patch
        1 kB
        Gregory Haskins

        Issue Links

          Activity

          Hide
          Bill Havanki added a comment -
          Show
          Bill Havanki added a comment - Review available: https://reviews.apache.org/r/23678/
          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.
          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.
          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?
          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
          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
          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.
          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) {
          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.

            People

            • Assignee:
              Bill Havanki
              Reporter:
              Gregory Haskins
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:

                Development