Hi Jim -

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

Thanks,

Henry

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

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

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

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.

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

Can you add a test for this? Thanks.

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

+1

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

Thanks!
--Michi

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

Checked in to 3.3, 3.4, and trunk.

--Michi

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