Uploaded image for project: 'ZooKeeper'
  1. ZooKeeper
  2. ZOOKEEPER-1318

In Python binding, get_children (and get and exists, and probably others) with expired session doesn't raise exception properly

Details

    • Bug
    • Status: Resolved
    • Major
    • Resolution: Fixed
    • 3.3.3
    • 3.3.6, 3.4.4, 3.5.0
    • contrib-bindings
    • None
    • Mac OS X (at least)

    • Reviewed

    Description

      In Python binding, get_children (and get and exists, and probably others) with expired session doesn't raise exception properly.

      >>> zookeeper.state(h)
      -112
      >>> zookeeper.get_children(h, '/')
      Traceback (most recent call last):
      File "<console>", line 1, in <module>
      SystemError: error return without exception set

      Let me know if you'd like me to work on a patch.

      Attachments

        1. ZOOKEEPER-1318.patch
          0.7 kB
          Henry Robinson
        2. ZOOKEEPER-1318.patch
          0.9 kB
          Henry Robinson

        Activity

          hudson Hudson added a comment -

          Integrated in ZooKeeper-trunk #1552 (See https://builds.apache.org/job/ZooKeeper-trunk/1552/)
          ZOOKEEPER-1318. In Python binding, get_children (and get and exists, and probably others) with expired session doesn't raise exception properly (henryr via michim) (Revision 1336467)

          Result = SUCCESS
          michim : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1336467
          Files :

          • /zookeeper/trunk/CHANGES.txt
          • /zookeeper/trunk/src/contrib/zkpython/src/c/zookeeper.c
          hudson Hudson added a comment - Integrated in ZooKeeper-trunk #1552 (See https://builds.apache.org/job/ZooKeeper-trunk/1552/ ) ZOOKEEPER-1318 . In Python binding, get_children (and get and exists, and probably others) with expired session doesn't raise exception properly (henryr via michim) (Revision 1336467) Result = SUCCESS michim : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1336467 Files : /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/contrib/zkpython/src/c/zookeeper.c

          Checked in to 3.3, 3.4, and trunk.

          --Michi

          michim Michi Mutsuzaki added a comment - Checked in to 3.3, 3.4, and trunk. --Michi

          Yes, that seems fine to me given the constraints and that it was manually verified. +1 Thanks Michi.

          phunt Patrick D. Hunt added a comment - Yes, that seems fine to me given the constraints and that it was manually verified. +1 Thanks Michi.

          +1

          Henry's justification for not having a test seems reasonable to me. Pat, would it be ok if I commit this?

          Thanks!
          --Michi

          michim Michi Mutsuzaki added a comment - +1 Henry's justification for not having a test seems reasonable to me. Pat, would it be ok if I commit this? Thanks! --Michi
          henryr Henry Robinson added a comment -

          Although a test would be ideal, as I mentioned above:

          • SessionExpired tests are tricky to write
          • This patch is trivial
          • We don't have coverage of this type ('does the correct exception get thrown') in most other places for zkpython
          henryr Henry Robinson added a comment - Although a test would be ideal, as I mentioned above: SessionExpired tests are tricky to write This patch is trivial We don't have coverage of this type ('does the correct exception get thrown') in most other places for zkpython

          Can you add a test for this? Thanks.

          phunt Patrick D. Hunt added a comment - Can you add a test for this? Thanks.
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12525110/ZOOKEEPER-1318.patch
          against trunk revision 1331246.

          +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 new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +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 (version 1.3.9) warnings.

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

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

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1055//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1055//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1055//console

          This message is automatically generated.

          hadoopqa Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12525110/ZOOKEEPER-1318.patch against trunk revision 1331246. +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 new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +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 (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1055//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1055//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1055//console This message is automatically generated.
          henryr Henry Robinson added a comment -

          Updated patch - InvalidStateException was already declared, just not dealt with in err_to_exception.

          This is a very simple patch, and tests are hard to write for session expired exceptions; also we don't have coverage for similar cases with other exceptions.

          henryr Henry Robinson added a comment - Updated patch - InvalidStateException was already declared, just not dealt with in err_to_exception. This is a very simple patch, and tests are hard to write for session expired exceptions; also we don't have coverage for similar cases with other exceptions.
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12525100/ZOOKEEPER-1318.patch
          against trunk revision 1331246.

          +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 new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +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 (version 1.3.9) warnings.

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

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

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1054//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1054//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1054//console

          This message is automatically generated.

          hadoopqa Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12525100/ZOOKEEPER-1318.patch against trunk revision 1331246. +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 new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +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 (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1054//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1054//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1054//console This message is automatically generated.
          henryr Henry Robinson added a comment -

          The error is a missing InvalidStateException. I've added the exception type, and confirmed that it show up when session expiration occurs.

          henryr Henry Robinson added a comment - The error is a missing InvalidStateException. I've added the exception type, and confirmed that it show up when session expiration occurs.

          Seems a bit quiet here Error happens due to missing case in err_to_exception function. What exception type should be set?

          nekto0n Nikita Vetoshkin added a comment - Seems a bit quiet here Error happens due to missing case in err_to_exception function. What exception type should be set?
          henryr Henry Robinson added a comment -

          Hi Jim -

          Good catch! By all means, feel free to work on a patch

          Thanks,

          Henry

          henryr Henry Robinson added a comment - Hi Jim - Good catch! By all means, feel free to work on a patch Thanks, Henry

          People

            henryr Henry Robinson
            j1m Jim Fulton
            Votes:
            1 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: