Details

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.

Activity

(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
added a comment - 23/Sep/09 18:12 (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.

Patrick Hunt
added a comment - 23/Sep/09 18:16 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! )

Patrick Hunt
added a comment - 02/Oct/09 17:04 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?

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.

Henry Robinson
added a comment - 06/Oct/09 00:47 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.

Henry Robinson
added a comment - 08/Oct/09 23:15 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.

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
added a comment - 09/Oct/09 04:40 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?

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?

Henry Robinson
added a comment - 09/Oct/09 08:06 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?

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.

Henry Robinson
added a comment - 13/Oct/09 06:58 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.

Hadoop QA
added a comment - 14/Oct/09 00:06 -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
added a comment - 14/Oct/09 00:14 +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)