Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.2.0
    • Fix Version/s: 3.2.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Python bindings for ZooKeeper. See src/contrib/zkpython/README for details.

      Description

      ZooKeeper doesn't have Python bindings. Having them would be useful, and would complement the extant Perl bindings.

      1. ZOOKEEPER-395.patch
        57 kB
        Mahadev konar
      2. ZOOKEEPER-395.patch
        57 kB
        Henry Robinson
      3. ZOOKEEPER-395.patch
        57 kB
        Henry Robinson
      4. ZOOKEEPER-395.patch
        50 kB
        Henry Robinson
      5. ZOOKEEPER-395.patch
        36 kB
        Henry Robinson

        Issue Links

          Activity

          Hide
          Henry Robinson added a comment -

          Here's a patch that adds Python bindings for the ZooKeeper C client. This is very early work and comes with attendant warnings about feature incompleteness and bugs.

          Instructions for building and installing are in the enclosed README. Watchers (should) work as the Python C API make it easy to asynchronously call back into the interpreter.

          For example:

          ZOO_OPEN_ACL_UNSAFE =

          {"perms":0x1f, "scheme":"world", "id" :"anyone"}

          ;

          if zookeeper.exists(handle, "/python-test", None) == None:
          print zookeeper.create(handle,"/python-test","random",[ZOO_OPEN_ACL_UNSAFE],0)
          else:
          print zookeeper.get_acl(handle,"/python-test")

          There are plenty of things that need improving (again see the README), but I'm offering this early for comment and in the hope of it being useful.

          Show
          Henry Robinson added a comment - Here's a patch that adds Python bindings for the ZooKeeper C client. This is very early work and comes with attendant warnings about feature incompleteness and bugs. Instructions for building and installing are in the enclosed README. Watchers (should) work as the Python C API make it easy to asynchronously call back into the interpreter. For example: ZOO_OPEN_ACL_UNSAFE = {"perms":0x1f, "scheme":"world", "id" :"anyone"} ; if zookeeper.exists(handle, "/python-test", None) == None: print zookeeper.create(handle,"/python-test","random", [ZOO_OPEN_ACL_UNSAFE] ,0) else: print zookeeper.get_acl(handle,"/python-test") There are plenty of things that need improving (again see the README), but I'm offering this early for comment and in the hope of it being useful.
          Hide
          Patrick Hunt added a comment -

          Wow, this is great! Thanks Henry.

          I was able to successfully build and attach to a running zookeeper server (linux).

          I looked at doing a python binding recently but I'm afraid while I'm a decent python programmer I don't know
          a thing about the python/c/c++ binding. I see you used the direct layer (rather than pyrex or something), that's
          hard core.

          We'd love to include this. If you could resubmit the patch with:

          1) put it into the src/contrib/zkpython directory (perl is in zkperl)

          2) c++ probably in src/contrib/zkpython/src/c++
          3) python probably in src/contrib/zkpython/src/python
          4) python based tests probably in src/contrib/zkpython/src/test
          5) README.txt in src/contrib/zkpython

          Show
          Patrick Hunt added a comment - Wow, this is great! Thanks Henry. I was able to successfully build and attach to a running zookeeper server (linux). I looked at doing a python binding recently but I'm afraid while I'm a decent python programmer I don't know a thing about the python/c/c++ binding. I see you used the direct layer (rather than pyrex or something), that's hard core. We'd love to include this. If you could resubmit the patch with: 1) put it into the src/contrib/zkpython directory (perl is in zkperl) 2) c++ probably in src/contrib/zkpython/src/c++ 3) python probably in src/contrib/zkpython/src/python 4) python based tests probably in src/contrib/zkpython/src/test 5) README.txt in src/contrib/zkpython
          Hide
          Henry Robinson added a comment -

          Here's a new patch conforming to the directory layout as suggested.

          Other improvements: the asynchronous API is now implemented, a nasty reference count bug has been fixed and most of the integer constants from zookeeper.h are now in the module. src/contrib/zkpython/src/python/zk.py demonstrates some of the working features.

          There are no tests yet - that's next on my to do list, followed by docstrings.

          Show
          Henry Robinson added a comment - Here's a new patch conforming to the directory layout as suggested. Other improvements: the asynchronous API is now implemented, a nasty reference count bug has been fixed and most of the integer constants from zookeeper.h are now in the module. src/contrib/zkpython/src/python/zk.py demonstrates some of the working features. There are no tests yet - that's next on my to do list, followed by docstrings.
          Hide
          Hadoop QA added a comment -

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

          +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 generated 159 release audit warnings (more than the trunk's current 156 warnings).

          -1 core tests. The patch failed core unit tests.

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

          Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/67/testReport/
          Release audit warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/67/artifact/trunk/current/releaseAuditDiffWarnings.txt
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/67/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/67/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/12407751/ZOOKEEPER-395.patch against trunk revision 772843. +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 generated 159 release audit warnings (more than the trunk's current 156 warnings). -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/67/testReport/ Release audit warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/67/artifact/trunk/current/releaseAuditDiffWarnings.txt Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/67/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/67/console This message is automatically generated.
          Hide
          Flavio Junqueira added a comment -

          Henry, you have to add the license header to the new files, otherwise the release audit test will fail.

          Show
          Flavio Junqueira added a comment - Henry, you have to add the license header to the new files, otherwise the release audit test will fail.
          Hide
          Henry Robinson added a comment -

          Added license headers.

          Show
          Henry Robinson added a comment - Added license headers.
          Hide
          Henry Robinson added a comment -

          License headers.

          Show
          Henry Robinson added a comment - License headers.
          Hide
          Patrick Hunt added a comment -

          Great! A few fyis:

          1) hadoopqabot only triggers on submit, so if you want to re-run the static verifications cancel and resubmit the patch after attaching the new patch file

          2) if you update a patch just attach with the same name - jira will be able to track the changes that way (the latest file, if dup names, will
          be highlighted). don't delete the old patch (in general)

          3) why not consider zk.py your smoketest/test? Move it into the test directory, we'll work out the build.xml changes necessary to build/test/package next. including adding the ability to run unit/func tests from build.xml.

          Show
          Patrick Hunt added a comment - Great! A few fyis: 1) hadoopqabot only triggers on submit, so if you want to re-run the static verifications cancel and resubmit the patch after attaching the new patch file 2) if you update a patch just attach with the same name - jira will be able to track the changes that way (the latest file, if dup names, will be highlighted). don't delete the old patch (in general) 3) why not consider zk.py your smoketest/test? Move it into the test directory, we'll work out the build.xml changes necessary to build/test/package next. including adding the ability to run unit/func tests from build.xml.
          Hide
          Henry Robinson added a comment -

          Thanks for the info - as you can tell I'm still getting up to speed with the Jira workflow and am still pretty clumsy with it. Thanks for your patience!

          3) Will do, although I might not have the chance to do so until this evening. I was planning to write a suite of unittest-based tests as well, so I'll add those to test and figure out the build.xml incantations at the same time.

          Show
          Henry Robinson added a comment - Thanks for the info - as you can tell I'm still getting up to speed with the Jira workflow and am still pretty clumsy with it. Thanks for your patience! 3) Will do, although I might not have the chance to do so until this evening. I was planning to write a suite of unittest-based tests as well, so I'll add those to test and figure out the build.xml incantations at the same time.
          Hide
          Patrick Hunt added a comment -

          No worries. We appreciate you taking the time to contribute back to the community!

          Either way you handle 3 is fine w/me.

          Show
          Patrick Hunt added a comment - No worries. We appreciate you taking the time to contribute back to the community! Either way you handle 3 is fine w/me.
          Hide
          Hadoop QA added a comment -

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

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/70/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/70/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/70/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/12407797/ZOOKEEPER-395.patch against trunk revision 772843. +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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/70/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/70/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/70/console This message is automatically generated.
          Hide
          Mahadev konar added a comment -

          henry,
          are you going to add tests to this patch?

          Show
          Mahadev konar added a comment - henry, are you going to add tests to this patch?
          Hide
          Henry Robinson added a comment -

          Yes they're in hand - I didn't get much time to work on them yesterday but I've knocked a couple off today.

          Show
          Henry Robinson added a comment - Yes they're in hand - I didn't get much time to work on them yesterday but I've knocked a couple off today.
          Hide
          Henry Robinson added a comment -

          This version contains tests for create, get, set, exists and delete code paths. The rest to follow as quickly as I can, but I don't want to hold up this patch trying to get all the unit tests finished if it can go ahead without.

          Also, this patch contains some important bugfixes (tests are good for finding those...) and a change to the way errors are handled. See zkpython/README for details.

          I haven't yet been able to figure out the right way to run python unittests from build.xml. Any ideas?

          Show
          Henry Robinson added a comment - This version contains tests for create, get, set, exists and delete code paths. The rest to follow as quickly as I can, but I don't want to hold up this patch trying to get all the unit tests finished if it can go ahead without. Also, this patch contains some important bugfixes (tests are good for finding those...) and a change to the way errors are handled. See zkpython/README for details. I haven't yet been able to figure out the right way to run python unittests from build.xml. Any ideas?
          Hide
          Mahadev konar added a comment -

          for the tests, you could write a shell script that would run all the python unit tests and exec that script from the build.xml. would that work?

          also, is their any requirements for python versions?

          Show
          Mahadev konar added a comment - for the tests, you could write a shell script that would run all the python unit tests and exec that script from the build.xml. would that work? also, is their any requirements for python versions?
          Hide
          Patrick Hunt added a comment -

          shell script would probably be the simplest. I did notice:
          http://code.google.com/p/pyanttasks/ specifically the py-test task
          but that would require users to copy the ant task to their ant directory, that might
          be ok, pyanttasks is under apache license so it could be included in the rel, also
          build.xml itself could have a "test-setup" target that copies pyanttasks into ant install
          directory.

          regardless you'll need to start/stop the server in order to run the python tests. Take a look
          at the rest contrib for an example of what I did for that.

          Show
          Patrick Hunt added a comment - shell script would probably be the simplest. I did notice: http://code.google.com/p/pyanttasks/ specifically the py-test task but that would require users to copy the ant task to their ant directory, that might be ok, pyanttasks is under apache license so it could be included in the rel, also build.xml itself could have a "test-setup" target that copies pyanttasks into ant install directory. regardless you'll need to start/stop the server in order to run the python tests. Take a look at the rest contrib for an example of what I did for that.
          Hide
          Mahadev konar added a comment -

          henry,
          I tried installing the python module to see how it works but I am getting this error while building

          src/c/zookeeper.c:19:20: Python.h: No such file or directory
          src/c/zookeeper.c:31: error: syntax error before "PyObject"
          

          and it fails to compile with lot more errors.

          This looks like some headers not availiable with my python setup. I have python 2.3 installed. Do I need to install some version greater than 2.3? or some other python module?

          Show
          Mahadev konar added a comment - henry, I tried installing the python module to see how it works but I am getting this error while building src/c/zookeeper.c:19:20: Python.h: No such file or directory src/c/zookeeper.c:31: error: syntax error before "PyObject" and it fails to compile with lot more errors. This looks like some headers not availiable with my python setup. I have python 2.3 installed. Do I need to install some version greater than 2.3? or some other python module?
          Hide
          Mahadev konar added a comment -

          ok I figured it out, I had to install python-devel package.

          Show
          Mahadev konar added a comment - ok I figured it out, I had to install python-devel package.
          Hide
          Henry Robinson added a comment -

          Hi Mahadev,

          Yes, you'll need the Python development headers installed. For yum-based systems these are packaged in python-devel. These are a dependency for building the module, but not installing it. I'm not sure if adding ant rules to automatically download the package is the right thing to do, as people often like to manage those things themselves, and version matching will be complex. I should at least add a warning to the readme.

          I think that the module will build correctly against 2.3 (and I'll be really interested to hear about your experiences when you get Python.h installed), however it certainly won't build against anything earlier than that (as PyGIL* were introduced in 2.3 which we need to get callbacks working). I don't use many fancy features, so I hope to be safe. I've built it against 2.5.1 here, and have a 2.6 system I can try. I expect 3.0 might have changed the API though, so I doubt the module will work there.

          Show
          Henry Robinson added a comment - Hi Mahadev, Yes, you'll need the Python development headers installed. For yum-based systems these are packaged in python-devel. These are a dependency for building the module, but not installing it. I'm not sure if adding ant rules to automatically download the package is the right thing to do, as people often like to manage those things themselves, and version matching will be complex. I should at least add a warning to the readme. I think that the module will build correctly against 2.3 (and I'll be really interested to hear about your experiences when you get Python.h installed), however it certainly won't build against anything earlier than that (as PyGIL* were introduced in 2.3 which we need to get callbacks working). I don't use many fancy features, so I hope to be safe. I've built it against 2.5.1 here, and have a 2.6 system I can try. I expect 3.0 might have changed the API though, so I doubt the module will work there.
          Hide
          Mahadev konar added a comment -

          yes, adding the warning in README would be good. We dont have to do all the installation in ant but just specifying what to install and what version of python to use is good enough to have in README. Also, it will be good to have it in the README saying whcih versions of python's we have tried running successfully against.

          Also just a minor change in README.

          python setup.py build
          sudo python setup.py install
          
          from zookeeper/src/contrib/zkpython/.
          

          should be

          python src/python/setup.py build
          sudo python src/python/setup.py install
          
          from zookeeper/src/contrib/zkpython/.
          
          Show
          Mahadev konar added a comment - yes, adding the warning in README would be good. We dont have to do all the installation in ant but just specifying what to install and what version of python to use is good enough to have in README. Also, it will be good to have it in the README saying whcih versions of python's we have tried running successfully against. Also just a minor change in README. python setup.py build sudo python setup.py install from zookeeper/src/contrib/zkpython/. should be python src/python/setup.py build sudo python src/python/setup.py install from zookeeper/src/contrib/zkpython/.
          Hide
          Mahadev konar added a comment -

          also, the library created should have a name like zookeeper_py.so ... so that users do not get confused by the name zookeeper.so.

          Show
          Mahadev konar added a comment - also, the library created should have a name like zookeeper_py.so ... so that users do not get confused by the name zookeeper.so.
          Hide
          Patrick Hunt added a comment -

          Mahadev, I believe the lib name is linked to the "import xxx" that's done in the python code. (user code currently),
          which is probably why Henry did it this way.

          However we will be putting an OO wrapper anyway, right? So perhaps renaming isn't such a big deal...?

          Show
          Patrick Hunt added a comment - Mahadev, I believe the lib name is linked to the "import xxx" that's done in the python code. (user code currently), which is probably why Henry did it this way. However we will be putting an OO wrapper anyway, right? So perhaps renaming isn't such a big deal...?
          Hide
          Henry Robinson added a comment -

          You're right - I think Python searches for modules by filename (certainly when I just tried renaming it, it would no longer import, although there may be more going on behind the scenes), so we would have to write import zookeeper_py which is a bit clunky to me. zookeeper.so goes in the local Python/2.5/site-packages directory on my machine which might make the differentiation clear. Even if we wrap it, it seems like we'd want some module to be called zookeeper eventually

          Show
          Henry Robinson added a comment - You're right - I think Python searches for modules by filename (certainly when I just tried renaming it, it would no longer import, although there may be more going on behind the scenes), so we would have to write import zookeeper_py which is a bit clunky to me. zookeeper.so goes in the local Python/2.5/site-packages directory on my machine which might make the differentiation clear. Even if we wrap it, it seems like we'd want some module to be called zookeeper eventually
          Hide
          Henry Robinson added a comment -

          This patch adds the beginnings of a testing infrastructure - the module is now built and tested via ant. Updated instructions are in the README. Thanks Mahadev for the useful feedback, most of which I've reflected in the README.

          I have gone for a simple shell script at this point, but I reckon in the future we should move to either nose or pyant to run the tests.

          The testing harness starts by starting a local server as in the rest module - thanks Patrick for pointing me at this.

          Both the server script and the test script (zkServer.sh and run_tests.sh in zkpython/src/test) need their executable bits set which is not preserved by this patch. Until they are chmodded, ant will not be able to run the tests correctly.

          Show
          Henry Robinson added a comment - This patch adds the beginnings of a testing infrastructure - the module is now built and tested via ant. Updated instructions are in the README. Thanks Mahadev for the useful feedback, most of which I've reflected in the README. I have gone for a simple shell script at this point, but I reckon in the future we should move to either nose or pyant to run the tests. The testing harness starts by starting a local server as in the rest module - thanks Patrick for pointing me at this. Both the server script and the test script (zkServer.sh and run_tests.sh in zkpython/src/test) need their executable bits set which is not preserved by this patch. Until they are chmodded, ant will not be able to run the tests correctly.
          Hide
          Mahadev konar added a comment -

          yeah, I think its fine in 2.*/site-packages.

          I tried running zk.py against a local zk server and this is what I got

          onnecting to localhost:2181 --
          Connected, handle is  0
          Creating sequence node  0   0
          Creating sequence node  1   0
          Creating sequence node  2   0
          Creating sequence node  3   0
          Creating sequence node  4   0
          ZNode tree --
          |---/ ::
                  |---/zk-python :: data
                          |---/zk-python/sequencenode0000000003 :: data
                          |---/zk-python/sequencenode0000000004 :: data
                          |---/zk-python/sequencenode0000000001 :: data
                          |---/zk-python/sequencenode0000000002 :: data
                          |---/zk-python/sequencenode0000000000 :: data
          Segmentation fault
          

          is zk.py working for you henry?

          Show
          Mahadev konar added a comment - yeah, I think its fine in 2.*/site-packages. I tried running zk.py against a local zk server and this is what I got onnecting to localhost:2181 -- Connected, handle is 0 Creating sequence node 0 0 Creating sequence node 1 0 Creating sequence node 2 0 Creating sequence node 3 0 Creating sequence node 4 0 ZNode tree -- |---/ :: |---/zk-python :: data |---/zk-python/sequencenode0000000003 :: data |---/zk-python/sequencenode0000000004 :: data |---/zk-python/sequencenode0000000001 :: data |---/zk-python/sequencenode0000000002 :: data |---/zk-python/sequencenode0000000000 :: data Segmentation fault is zk.py working for you henry?
          Hide
          Henry Robinson added a comment -

          Yes, it works for me - looks like it's crashing for you halfway through the ZK tree, probably when it's reading the /zookeeper directory. Is this a clean server? Running against the latest module version (0.3), using python 2.3?

          Show
          Henry Robinson added a comment - Yes, it works for me - looks like it's crashing for you halfway through the ZK tree, probably when it's reading the /zookeeper directory. Is this a clean server? Running against the latest module version (0.3), using python 2.3?
          Hide
          Mahadev konar added a comment -

          hmm that was it... it wasnt a clean server ... I wonder why it just crashed... I mgiht have had a version of zk database I was doing some testing on ...

          Show
          Mahadev konar added a comment - hmm that was it... it wasnt a clean server ... I wonder why it just crashed... I mgiht have had a version of zk database I was doing some testing on ...
          Hide
          Todd Lipcon added a comment -

          Both the server script and the test script (zkServer.sh and run_tests.sh in zkpython/src/test) need their executable bits set which is not preserved by this patch. Until they are chmodded, ant will not be able to run the tests correctly.

          Can you add a chmod task to the build.xml? Something like the following:

          <target name="chmod-shell-scripts">
            <chmod perm="ugo+x" file="path/to/zkServer.sh" />
            <chmod perm="ugo+x" file="path/to/run_tests.sh" />
          </target>
          

          and then make the test target depend on it?

          Show
          Todd Lipcon added a comment - Both the server script and the test script (zkServer.sh and run_tests.sh in zkpython/src/test) need their executable bits set which is not preserved by this patch. Until they are chmodded, ant will not be able to run the tests correctly. Can you add a chmod task to the build.xml? Something like the following: <target name= "chmod-shell-scripts" > <chmod perm= "ugo+x" file= "path/to/zkServer.sh" /> <chmod perm= "ugo+x" file= "path/to/run_tests.sh" /> </target> and then make the test target depend on it?
          Hide
          Henry Robinson added a comment -

          Still, I'd like to know why it crashed - do you still have the snapshot lying around? (and if so, is it possible to send my way for me to start a server against?)

          If the crash was in the C library then that's ok (from my perspective ) as it sounds like you had changed something. If it crashed in the module that's a worry.

          Show
          Henry Robinson added a comment - Still, I'd like to know why it crashed - do you still have the snapshot lying around? (and if so, is it possible to send my way for me to start a server against?) If the crash was in the C library then that's ok (from my perspective ) as it sounds like you had changed something. If it crashed in the module that's a worry.
          Hide
          Mahadev konar added a comment -

          yes I do have the snapshot and am actually trying to see why it failed. Ill update on the jira in some time...

          Show
          Mahadev konar added a comment - yes I do have the snapshot and am actually trying to see why it failed. Ill update on the jira in some time...
          Hide
          Patrick Hunt added a comment -

          chmod in build.xml shouldn't be necessary - the committer will set the exec flag before committing.

          NOTE to comitter: be sure to set the exec flag.

          Show
          Patrick Hunt added a comment - chmod in build.xml shouldn't be necessary - the committer will set the exec flag before committing. NOTE to comitter: be sure to set the exec flag.
          Hide
          Patrick Hunt added a comment -

          +1 – sounds to me like this is ready to commit.

          are there any issues that must be addressed before commit?

          Show
          Patrick Hunt added a comment - +1 – sounds to me like this is ready to commit. are there any issues that must be addressed before commit?
          Hide
          Henry Robinson added a comment -

          This is my to-do list (not in order of priority):

          1. Finish test coverage
          2. Move from shell script to pyant / nose for testing
          3. Test against Python 2.6, investigate 3.0
          4. Make watcher arguments optional
          5. Design and implement a wrapper object
          6. Add a way to get the numeric error code of the last call made (as failures are exposed as exceptions with string messages)
          7. Add docstrings to the module

          None of these seem serious enough to block the commit, in my opinion. I think the most important one is probably to test against 2.6 and I'll do that soon.

          Show
          Henry Robinson added a comment - This is my to-do list (not in order of priority): 1. Finish test coverage 2. Move from shell script to pyant / nose for testing 3. Test against Python 2.6, investigate 3.0 4. Make watcher arguments optional 5. Design and implement a wrapper object 6. Add a way to get the numeric error code of the last call made (as failures are exposed as exceptions with string messages) 7. Add docstrings to the module None of these seem serious enough to block the commit, in my opinion. I think the most important one is probably to test against 2.6 and I'll do that soon.
          Hide
          Mahadev konar added a comment -

          well, it turns out that the seg fault is a bug in c library ZOOKEEPER-402. .

          Show
          Mahadev konar added a comment - well, it turns out that the seg fault is a bug in c library ZOOKEEPER-402 . .
          Hide
          Mahadev konar added a comment -

          yeah the patch looks good. .. Ill take a final look at it and commit it later today....

          Show
          Mahadev konar added a comment - yeah the patch looks good. .. Ill take a final look at it and commit it later today....
          Hide
          Henry Robinson added a comment -

          Here's a minor fix as a result of testing against 2.6 - the log file stream was not being correctly reference counted causing a segfault on the GC in 2.6 which is apparently different to 2.5. The README is updated to mention the testing, which all passed after that fix.

          Show
          Henry Robinson added a comment - Here's a minor fix as a result of testing against 2.6 - the log file stream was not being correctly reference counted causing a segfault on the GC in 2.6 which is apparently different to 2.5. The README is updated to mention the testing, which all passed after that fix.
          Hide
          Mahadev konar added a comment -

          just modified the patch a little bit in build.xml to fail on any of the targets failing.

          Show
          Mahadev konar added a comment - just modified the patch a little bit in build.xml to fail on any of the targets failing.
          Hide
          Mahadev konar added a comment -

          I just committed this. thanks henry.

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

            People

            • Assignee:
              Henry Robinson
              Reporter:
              Henry Robinson
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development