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

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.3.3
    • Fix Version/s: 3.3.6, 3.4.4, 3.5.0
    • Component/s: contrib-bindings
    • Labels:
      None
    • Environment:

      Mac OS X (at least)

    • Hadoop Flags:
      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.

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

        Activity

        Hide
        Henry Robinson added a comment -

        Hi Jim -

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

        Thanks,

        Henry

        Show
        Henry Robinson added a comment - Hi Jim - Good catch! By all means, feel free to work on a patch Thanks, Henry
        Hide
        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?

        Show
        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?
        Hide
        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.

        Show
        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.
        Hide
        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.

        Show
        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.
        Hide
        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.

        Show
        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.
        Hide
        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.

        Show
        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.
        Hide
        Patrick Hunt added a comment -

        Can you add a test for this? Thanks.

        Show
        Patrick Hunt added a comment - Can you add a test for this? Thanks.
        Hide
        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
        Show
        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
        Hide
        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

        Show
        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
        Hide
        Patrick Hunt added a comment -

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

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

        Checked in to 3.3, 3.4, and trunk.

        --Michi

        Show
        Michi Mutsuzaki added a comment - Checked in to 3.3, 3.4, and trunk. --Michi
        Hide
        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
        Show
        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

          People

          • Assignee:
            Henry Robinson
            Reporter:
            Jim Fulton
          • Votes:
            1 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development