ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-510

zkpython lumps all exceptions as IOError, needs specialized exceptions for KeeperException types

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.2.0
    • Fix Version/s: 3.2.2, 3.3.0
    • Component/s: contrib-bindings
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      The current zkpython bindings always throw "IOError("text")" exceptions, even for ZK specific exceptions such as NODEEXISTS. This makes it difficult (error prone) to handle exceptions in python code. You can't easily pickup a connection loss vs a node exists for example. Of course you could match the error string, but this seems like a bad idea imo.

      We need to add specific exception types to the python binding that map directly to KeeperException/java types. It would also be useful to include the information provided by the KeeperException (like path in some cases), etc... as part of the error thrown to the python code. Would probably be a good idea to stay as close to java api as possible wrt mapping the errors.

      1. ZOOKEEPER-510.patch
        29 kB
        Henry Robinson
      2. ZOOKEEPER-510.patch
        16 kB
        Patrick Hunt
      3. ZOOKEEPER-510.patch
        16 kB
        Patrick Hunt
      4. ZOOKEEPER-510.patch
        12 kB
        Henry Robinson
      5. ZOOKEEPER-510-incref.patch
        0.8 kB
        Henry Robinson

        Activity

        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Patch Available Patch Available
        48d 49m 1 Henry Robinson 06/Oct/09 23:03
        Resolved Resolved Reopened Reopened
        3h 55m 1 Patrick Hunt 09/Oct/09 05:38
        Reopened Reopened Patch Available Patch Available
        4d 18h 11m 1 Patrick Hunt 13/Oct/09 23:49
        Patch Available Patch Available Resolved Resolved
        2d 4h 4m 2 Patrick Hunt 14/Oct/09 01:14
        Resolved Resolved Closed Closed
        163d 17h 10m 1 Patrick Hunt 26/Mar/10 17:25
        Patrick Hunt made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Patrick Hunt made changes -
        Fix Version/s 3.2.2 [ 12314335 ]
        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #497 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/497/)
        . zkpython lumps all exceptions as IOError, needs specialized exceptions for KeeperException types (applying the incremental patch)

        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #497 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/497/ ) . zkpython lumps all exceptions as IOError, needs specialized exceptions for KeeperException types (applying the incremental patch)
        Patrick Hunt made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Patrick Hunt added a comment -

        +1 looks good. We are testing the usual cases and implemented per the python docs, afaict there's no way to verify this further (although we have tests that cover this in general)

        Show
        Patrick Hunt added a comment - +1 looks good. We are testing the usual cases and implemented per the python docs, afaict there's no way to verify this further (although we have tests that cover this in general)
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12421957/ZOOKEEPER-510-incref.patch
        against trunk revision 824971.

        +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 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 passed core unit tests.

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

        Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/23/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/23/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/23/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/12421957/ZOOKEEPER-510-incref.patch against trunk revision 824971. +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 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 passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/23/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/23/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/23/console This message is automatically generated.
        Patrick Hunt made changes -
        Status Reopened [ 4 ] Patch Available [ 10002 ]
        Henry Robinson made changes -
        Attachment ZOOKEEPER-510-incref.patch [ 12421957 ]
        Hide
        Henry Robinson added a comment -

        Here's the patch for the incref issue. Try as I might, I haven't yet got a gc collection to dispose of the exception object and therefore trigger potential segfaults when the zookeeper module raises. So I haven't included any further tests. There are tests already that should exercise the paths prone to error if the behaviour of the gc does become more aggressive in future.

        Like I said previously, the reference that Patrick provides appears to be authoritative and explicitly describes this situation, so I think it's clearly correct to INCREF the exception objects.

        Show
        Henry Robinson added a comment - Here's the patch for the incref issue. Try as I might, I haven't yet got a gc collection to dispose of the exception object and therefore trigger potential segfaults when the zookeeper module raises. So I haven't included any further tests. There are tests already that should exercise the paths prone to error if the behaviour of the gc does become more aggressive in future. Like I said previously, the reference that Patrick provides appears to be authoritative and explicitly describes this situation, so I think it's clearly correct to INCREF the exception objects.
        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #493 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/493/)

        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #493 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/493/ )
        Hide
        Henry Robinson added a comment -

        Hm, I had interpreted the Python documentation differently, by analogy with PyModule_AddIntConstant. I also tested but couldn't force a GC error. This is what I just tried:

        >>> import zookeeper
        >>> import gc
        >>> gc.collect()
        0
        >>> zookeeper.set(0,"/","test")
        Traceback (most recent call last):
        File "<stdin>", line 1, in <module>
        zookeeper.ZooKeeperException: zhandle out of range

        I also tried removing the reference from the module before doing the gc, via del zookeeper.ZooKeeperException.

        However, I think you're right, by looking at other usage examples. I think it's acceptable to take the examples in the Python manual as canonical, so I support reinstating that line.

        The patch is simple, but I'm not sure how best to test it. Perhaps forcing an exception pointer to NULL via a C module call, but then that would pollute the object's namespace. A test module that wasn't shipped, but built for testing?

        Show
        Henry Robinson added a comment - Hm, I had interpreted the Python documentation differently, by analogy with PyModule_AddIntConstant. I also tested but couldn't force a GC error. This is what I just tried: >>> import zookeeper >>> import gc >>> gc.collect() 0 >>> zookeeper.set(0,"/","test") Traceback (most recent call last): File "<stdin>", line 1, in <module> zookeeper.ZooKeeperException: zhandle out of range I also tried removing the reference from the module before doing the gc, via del zookeeper.ZooKeeperException. However, I think you're right, by looking at other usage examples. I think it's acceptable to take the examples in the Python manual as canonical, so I support reinstating that line. The patch is simple, but I'm not sure how best to test it. Perhaps forcing an exception pointer to NULL via a C module call, but then that would pollute the object's namespace. A test module that wasn't shipped, but built for testing?
        Hide
        Patrick Hunt added a comment -

        I suggest creating an additional patch on this jira to address. Is this testable? the script could clear the exception from the module (it would be gc'd) then cause the code to raise the exception should cause seg fault?

        Show
        Patrick Hunt added a comment - I suggest creating an additional patch on this jira to address. Is this testable? the script could clear the exception from the module (it would be gc'd) then cause the code to raise the exception should cause seg fault?
        Patrick Hunt made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Hide
        Patrick Hunt added a comment -

        Unless I'm missing something there's a problem with this patch, the following is incorrect according to the python manual:

        +#define ADD_EXCEPTION x = PyErr_NewException("zookeeper."#x, ZooKeeperException, NULL); \
        + PyModule_AddObject(module, #x, x);

        specifically the ref is not being incremented.

        see this example in the following python man page:
        http://docs.python.org/extending/extending.html#intermezzo-errors-and-exceptions

        SpamError = PyErr_NewException("spam.error", NULL, NULL);
        Py_INCREF(SpamError);
        PyModule_AddObject(m, "error", SpamError);

        Show
        Patrick Hunt added a comment - Unless I'm missing something there's a problem with this patch, the following is incorrect according to the python manual: +#define ADD_EXCEPTION x = PyErr_NewException("zookeeper."#x, ZooKeeperException, NULL); \ + PyModule_AddObject(module, #x, x); specifically the ref is not being incremented. see this example in the following python man page: http://docs.python.org/extending/extending.html#intermezzo-errors-and-exceptions SpamError = PyErr_NewException("spam.error", NULL, NULL); Py_INCREF(SpamError); PyModule_AddObject(m, "error", SpamError);
        Mahadev konar made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Hadoop Flags [Reviewed]
        Resolution Fixed [ 1 ]
        Hide
        Mahadev konar added a comment -

        I just committed this. thanks henry!

        Show
        Mahadev konar added a comment - I just committed this. thanks henry!
        Hide
        Mahadev konar added a comment -

        great... missed it... my bad...

        Show
        Mahadev konar added a comment - great... missed it... my bad...
        Hide
        Henry Robinson added a comment -

        Yes - see testconnection in connection_test.py

        Two cases: trying to close an already closed handle, and trying to issue commands on an already closed handle. Both should raise exceptions.

        Show
        Henry Robinson added a comment - Yes - see testconnection in connection_test.py Two cases: trying to close an already closed handle, and trying to issue commands on an already closed handle. Both should raise exceptions.
        Hide
        Mahadev konar added a comment -

        the patch looks good ...
        henry, do we have a test case for ZOOKEEPER-540?

        Show
        Mahadev konar added a comment - the patch looks good ... henry, do we have a test case for ZOOKEEPER-540 ?
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12421360/ZOOKEEPER-510.patch
        against trunk revision 822065.

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

        +1 tests included. The patch appears to include 25 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 passed core unit tests.

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

        Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/19/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/19/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/19/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/12421360/ZOOKEEPER-510.patch against trunk revision 822065. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 25 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 passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/19/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/19/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/19/console This message is automatically generated.
        Henry Robinson made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Henry Robinson added a comment -

        I should mention that the segfault issue is already reported at ZOOKEEPER-540 - this patch should fix that issue and this one.

        Show
        Henry Robinson added a comment - I should mention that the segfault issue is already reported at ZOOKEEPER-540 - this patch should fix that issue and this one.
        Henry Robinson made changes -
        Attachment ZOOKEEPER-510.patch [ 12421360 ]
        Hide
        Henry Robinson added a comment -

        This patch adds exceptions, and expands the test suite to cover some commonly thrown exception cases.

        At the same time, it also includes a patch for the segfault issue when reusing a closed handle (since getting the exceptions right here requires fixing that issue, and fixing that issue requires proper exception testing).

        There were a couple of other bugs discovered during this process which this patch hopefully also squashes.

        Show
        Henry Robinson added a comment - This patch adds exceptions, and expands the test suite to cover some commonly thrown exception cases. At the same time, it also includes a patch for the segfault issue when reusing a closed handle (since getting the exceptions right here requires fixing that issue, and fixing that issue requires proper exception testing). There were a couple of other bugs discovered during this process which this patch hopefully also squashes.
        Hide
        Patrick Hunt added a comment -

        well what I did works, but plz double check that it's right. thanks.

        Show
        Patrick Hunt added a comment - well what I did works, but plz double check that it's right. thanks.
        Hide
        Henry Robinson added a comment -

        Hah, I'd just made the same patch. Good catch, thanks. Let me fix up some more tests and I'll upload patch for committing.

        Show
        Henry Robinson added a comment - Hah, I'd just made the same patch. Good catch, thanks. Let me fix up some more tests and I'll upload patch for committing.
        Patrick Hunt made changes -
        Attachment ZOOKEEPER-510.patch [ 12421155 ]
        Hide
        Patrick Hunt added a comment -

        updated patch, same as before, except that you need to add the exception to the module (not just create it)

        now passes the test I added

        Show
        Patrick Hunt added a comment - updated patch, same as before, except that you need to add the exception to the module (not just create it) now passes the test I added
        Patrick Hunt made changes -
        Attachment ZOOKEEPER-510.patch [ 12421124 ]
        Hide
        Patrick Hunt added a comment -

        this patch is the same as previous, except:

        1) fixes log output of test to goto trunk/build/contrib heirarchy

        2) adds a test for NodeExistsException

        Unfortunately this test fails.

        Henry, could you take a look? Am I doing something wrong here?

        Show
        Patrick Hunt added a comment - this patch is the same as previous, except: 1) fixes log output of test to goto trunk/build/contrib heirarchy 2) adds a test for NodeExistsException Unfortunately this test fails. Henry, could you take a look? Am I doing something wrong here?
        Henry Robinson made changes -
        Attachment ZOOKEEPER-510.patch [ 12421085 ]
        Hide
        Henry Robinson added a comment -

        This patch adds all the exceptions that the Java client has, and replaces all the IOErrors with zookeeper.ZooKeeperException or direct descendants.

        Show
        Henry Robinson added a comment - This patch adds all the exceptions that the Java client has, and replaces all the IOErrors with zookeeper.ZooKeeperException or direct descendants.
        Hide
        Patrick Hunt added a comment -

        sounds good. I didn't mean/intend that python had to mirror java exactly, just that the closer it is the easier for ppl to transition. Obv we
        should "pythonize" so that we fit std idioms, etc....

        (glad to have you back! )

        Show
        Patrick Hunt added a comment - sounds good. I didn't mean/intend that python had to mirror java exactly, just that the closer it is the easier for ppl to transition. Obv we should "pythonize" so that we fit std idioms, etc.... (glad to have you back! )
        Hide
        Henry Robinson added a comment -

        (Just got back from a long vacation, hence apologies for being slow to pick this up!)

        Yes, this is bad. zkpython needs to mirror the set of Java exceptions as closely as possible, and it shouldn't be hard to do.

        There are some special cases for when exceptions should be raised - checking for ret != ZOK doesn't always work. I'm thinking of exists, which (in ZooKeeper) special cases NONODE and returns null instead of a KeeperException. This seems sensible to me - just wanted to explicitly acknowledge that I'm using ZooKeeper.java as the reference for how a client API should behave since the API docs are vague on exactly when an exception is raised.

        Show
        Henry Robinson added a comment - (Just got back from a long vacation, hence apologies for being slow to pick this up!) Yes, this is bad. zkpython needs to mirror the set of Java exceptions as closely as possible, and it shouldn't be hard to do. There are some special cases for when exceptions should be raised - checking for ret != ZOK doesn't always work. I'm thinking of exists, which (in ZooKeeper) special cases NONODE and returns null instead of a KeeperException. This seems sensible to me - just wanted to explicitly acknowledge that I'm using ZooKeeper.java as the reference for how a client API should behave since the API docs are vague on exactly when an exception is raised.
        Henry Robinson made changes -
        Field Original Value New Value
        Assignee Henry Robinson [ henryr ]
        Patrick Hunt created issue -

          People

          • Assignee:
            Henry Robinson
            Reporter:
            Patrick Hunt
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development