ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1452

zoo_multi() & zoo_amulti() update operations for zkpython

    Details

    • Type: Improvement Improvement
    • Status: Patch Available
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: 3.5.0
    • Component/s: contrib-bindings
    • Labels:
    • Release Note:
      Hide
      Adds new `zoo_multi()` && `zoo_amulti()` functionality to the zkpython bindings for zookeeper.

      Includes a new unit test. Also used the functions from a python program that uses zkpython.

      All existing unit tests still pass.
      Show
      Adds new `zoo_multi()` && `zoo_amulti()` functionality to the zkpython bindings for zookeeper. Includes a new unit test. Also used the functions from a python program that uses zkpython. All existing unit tests still pass.
    • Tags:
      python, zkpython

      Description

      ZooKeeper's python bindings (src/contrib/zkpython) are missing multi-update support (zoo_multi() & zoo_amulti()) that was added to the C client recently. This issue is to bridge this gap, and add support for multi-update operations to the Python bindings.

      1. ZOOKEEPER-1452.patch
        39 kB
        Aravind Narayanan
      2. ZOOKEEPER-1452.patch
        39 kB
        Aravind Narayanan
      3. ZOOKEEPER-1452.patch
        42 kB
        Aravind Narayanan
      4. ZOOKEEPER-1452.patch
        39 kB
        Aravind Narayanan

        Activity

        Hide
        Aravind Narayanan added a comment -

        This patch adds new `zoo_multi()` and `zoo_amulti()` functionality to the zkpython bindings. It includes unit tests.

        In needs to be reviewed.

        Show
        Aravind Narayanan added a comment - This patch adds new `zoo_multi()` and `zoo_amulti()` functionality to the zkpython bindings. It includes unit tests. In needs to be reviewed.
        Hide
        Aravind Narayanan added a comment -

        This patch adds new `zoo_multi()` and `zoo_amulti()` functionality to the zkpython bindings. It includes unit tests.

        In needs to be reviewed.

        Show
        Aravind Narayanan added a comment - This patch adds new `zoo_multi()` and `zoo_amulti()` functionality to the zkpython bindings. It includes unit tests. In needs to be reviewed.
        Hide
        Hadoop QA added a comment -

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

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

        +1 tests included. The patch appears to include 4 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 (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/1046//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1046//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1046//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/12523538/ZOOKEEPER-1452.patch against trunk revision 1326029. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 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 (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/1046//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1046//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1046//console This message is automatically generated.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4958/
        -----------------------------------------------------------

        Review request for zookeeper and Henry Robinson.

        Summary
        -------

        Creating this review request on behalf of Aravind Narayanan for ZOOKEEPER-1452

        This addresses bug ZOOKEEPER-1452.
        https://issues.apache.org/jira/browse/ZOOKEEPER-1452

        Diffs


        CHANGES.txt 569fc12
        src/c/src/hashtable/hashtable_itr.h 234d3e3

        Diff: https://reviews.apache.org/r/4958/diff

        Testing
        -------

        Thanks,

        Henry

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4958/ ----------------------------------------------------------- Review request for zookeeper and Henry Robinson. Summary ------- Creating this review request on behalf of Aravind Narayanan for ZOOKEEPER-1452 This addresses bug ZOOKEEPER-1452 . https://issues.apache.org/jira/browse/ZOOKEEPER-1452 Diffs CHANGES.txt 569fc12 src/c/src/hashtable/hashtable_itr.h 234d3e3 Diff: https://reviews.apache.org/r/4958/diff Testing ------- Thanks, Henry
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4958/
        -----------------------------------------------------------

        (Updated 2012-05-01 22:22:51.531968)

        Review request for zookeeper and Henry Robinson.

        Changes
        -------

        Woops, wrong diff.

        Summary
        -------

        Creating this review request on behalf of Aravind Narayanan for ZOOKEEPER-1452

        This addresses bug ZOOKEEPER-1452.
        https://issues.apache.org/jira/browse/ZOOKEEPER-1452

        Diffs (updated)


        src/contrib/zkpython/README 89d9998
        src/contrib/zkpython/src/c/pyzk_docstrings.h d2c4d60
        src/contrib/zkpython/src/c/zookeeper.c 85fa512

        Diff: https://reviews.apache.org/r/4958/diff

        Testing
        -------

        Thanks,

        Henry

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4958/ ----------------------------------------------------------- (Updated 2012-05-01 22:22:51.531968) Review request for zookeeper and Henry Robinson. Changes ------- Woops, wrong diff. Summary ------- Creating this review request on behalf of Aravind Narayanan for ZOOKEEPER-1452 This addresses bug ZOOKEEPER-1452 . https://issues.apache.org/jira/browse/ZOOKEEPER-1452 Diffs (updated) src/contrib/zkpython/README 89d9998 src/contrib/zkpython/src/c/pyzk_docstrings.h d2c4d60 src/contrib/zkpython/src/c/zookeeper.c 85fa512 Diff: https://reviews.apache.org/r/4958/diff Testing ------- Thanks, Henry
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4958/#review7454
        -----------------------------------------------------------

        This is really great, thanks Aravind! Some comments below, most of them minor. I forgot to add the new test case (which looks good) to this review; I'll do that in the next iteration.

        src/contrib/zkpython/src/c/pyzk_docstrings.h
        <https://reviews.apache.org/r/4958/#comment16438>

        Typo: synchronously

        src/contrib/zkpython/src/c/pyzk_docstrings.h
        <https://reviews.apache.org/r/4958/#comment16439>

        Can you expand on this a bit - what kind of data structure should an 'op' be represented as in Python?

        src/contrib/zkpython/src/c/pyzk_docstrings.h
        <https://reviews.apache.org/r/4958/#comment16440>

        zoo_op_result_t isn't a visible type in Python (that's where these strings are visible). Can you rephrase in terms of something a Python programmer would understand?

        src/contrib/zkpython/src/c/zookeeper.c
        <https://reviews.apache.org/r/4958/#comment16441>

        Nit: char , not char

        src/contrib/zkpython/src/c/zookeeper.c
        <https://reviews.apache.org/r/4958/#comment16442>

        nit: Stat *stat

        src/contrib/zkpython/src/c/zookeeper.c
        <https://reviews.apache.org/r/4958/#comment16443>

        Can you add a comment indicating that this is the 'root' function through which all the other serialize methods are called? It wasn't obvious to me on the first pass why this did NULL checking but the other serialize_* methods didn't.

        src/contrib/zkpython/src/c/zookeeper.c
        <https://reviews.apache.org/r/4958/#comment16444>

        Nit: I think this should be PyExc_TypeError

        src/contrib/zkpython/src/c/zookeeper.c
        <https://reviews.apache.org/r/4958/#comment16445>

        This can return -1 on an error, and there's nothing preventing us from having some ZOO_*_OP == -1 in the future. Either use the non-error checking form PyInt_AS_LONG (if appropriate), or explicitly check for -1 before entering the switch:

        int type = (int)PyInt_AsLong(PyDict_GetItemString(pyop, "type");
        if (type == -1)

        { /* check PyErr_Occurred */ }

        switch (type)

        { ... }

        src/contrib/zkpython/src/c/zookeeper.c
        <https://reviews.apache.org/r/4958/#comment16447>

        If the serialize fails, do you need to free buf?

        src/contrib/zkpython/src/c/zookeeper.c
        <https://reviews.apache.org/r/4958/#comment16449>

        It's no big deal, but this could be more concise as:

        free(result[i].value);
        free(result[i].stat);

        src/contrib/zkpython/src/c/zookeeper.c
        <https://reviews.apache.org/r/4958/#comment16450>

        typo: amulti

        src/contrib/zkpython/src/c/zookeeper.c
        <https://reviews.apache.org/r/4958/#comment16451>

        Do you need to call PyGILState_Release here? What about Py_DECREF(pyresults)?

        src/contrib/zkpython/src/c/zookeeper.c
        <https://reviews.apache.org/r/4958/#comment16454>

        Note: formatting of switch statements is inconsistent with what already existed. The parentheses around each case block are extraneous, and there's an extra level of indentation. Feel free to fix the switch in err_to_exception to have the same indentation, but you can probably lose the { } on your case blocks.

        src/contrib/zkpython/src/c/zookeeper.c
        <https://reviews.apache.org/r/4958/#comment16455>

        "Completion parameter must be callable"

        src/contrib/zkpython/src/c/zookeeper.c
        <https://reviews.apache.org/r/4958/#comment16456>

        I think you need to free_ops here, and arguably in all the other error cases (admittedly, if malloc is returning NULL, it's going to be hard to recover)

        • Henry

        On 2012-05-01 22:22:51, Henry Robinson wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4958/

        -----------------------------------------------------------

        (Updated 2012-05-01 22:22:51)

        Review request for zookeeper and Henry Robinson.

        Summary

        -------

        Creating this review request on behalf of Aravind Narayanan for ZOOKEEPER-1452

        This addresses bug ZOOKEEPER-1452.

        https://issues.apache.org/jira/browse/ZOOKEEPER-1452

        Diffs

        -----

        src/contrib/zkpython/README 89d9998

        src/contrib/zkpython/src/c/pyzk_docstrings.h d2c4d60

        src/contrib/zkpython/src/c/zookeeper.c 85fa512

        Diff: https://reviews.apache.org/r/4958/diff

        Testing

        -------

        Thanks,

        Henry

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4958/#review7454 ----------------------------------------------------------- This is really great, thanks Aravind! Some comments below, most of them minor. I forgot to add the new test case (which looks good) to this review; I'll do that in the next iteration. src/contrib/zkpython/src/c/pyzk_docstrings.h < https://reviews.apache.org/r/4958/#comment16438 > Typo: synchronously src/contrib/zkpython/src/c/pyzk_docstrings.h < https://reviews.apache.org/r/4958/#comment16439 > Can you expand on this a bit - what kind of data structure should an 'op' be represented as in Python? src/contrib/zkpython/src/c/pyzk_docstrings.h < https://reviews.apache.org/r/4958/#comment16440 > zoo_op_result_t isn't a visible type in Python (that's where these strings are visible). Can you rephrase in terms of something a Python programmer would understand? src/contrib/zkpython/src/c/zookeeper.c < https://reviews.apache.org/r/4958/#comment16441 > Nit: char , not char src/contrib/zkpython/src/c/zookeeper.c < https://reviews.apache.org/r/4958/#comment16442 > nit: Stat *stat src/contrib/zkpython/src/c/zookeeper.c < https://reviews.apache.org/r/4958/#comment16443 > Can you add a comment indicating that this is the 'root' function through which all the other serialize methods are called? It wasn't obvious to me on the first pass why this did NULL checking but the other serialize_* methods didn't. src/contrib/zkpython/src/c/zookeeper.c < https://reviews.apache.org/r/4958/#comment16444 > Nit: I think this should be PyExc_TypeError src/contrib/zkpython/src/c/zookeeper.c < https://reviews.apache.org/r/4958/#comment16445 > This can return -1 on an error, and there's nothing preventing us from having some ZOO_*_OP == -1 in the future. Either use the non-error checking form PyInt_AS_LONG (if appropriate), or explicitly check for -1 before entering the switch: int type = (int)PyInt_AsLong(PyDict_GetItemString(pyop, "type"); if (type == -1) { /* check PyErr_Occurred */ } switch (type) { ... } src/contrib/zkpython/src/c/zookeeper.c < https://reviews.apache.org/r/4958/#comment16447 > If the serialize fails, do you need to free buf? src/contrib/zkpython/src/c/zookeeper.c < https://reviews.apache.org/r/4958/#comment16449 > It's no big deal, but this could be more concise as: free(result [i] .value); free(result [i] .stat); src/contrib/zkpython/src/c/zookeeper.c < https://reviews.apache.org/r/4958/#comment16450 > typo: amulti src/contrib/zkpython/src/c/zookeeper.c < https://reviews.apache.org/r/4958/#comment16451 > Do you need to call PyGILState_Release here? What about Py_DECREF(pyresults)? src/contrib/zkpython/src/c/zookeeper.c < https://reviews.apache.org/r/4958/#comment16454 > Note: formatting of switch statements is inconsistent with what already existed. The parentheses around each case block are extraneous, and there's an extra level of indentation. Feel free to fix the switch in err_to_exception to have the same indentation, but you can probably lose the { } on your case blocks. src/contrib/zkpython/src/c/zookeeper.c < https://reviews.apache.org/r/4958/#comment16455 > "Completion parameter must be callable" src/contrib/zkpython/src/c/zookeeper.c < https://reviews.apache.org/r/4958/#comment16456 > I think you need to free_ops here, and arguably in all the other error cases (admittedly, if malloc is returning NULL, it's going to be hard to recover) Henry On 2012-05-01 22:22:51, Henry Robinson wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4958/ ----------------------------------------------------------- (Updated 2012-05-01 22:22:51) Review request for zookeeper and Henry Robinson. Summary ------- Creating this review request on behalf of Aravind Narayanan for ZOOKEEPER-1452 This addresses bug ZOOKEEPER-1452 . https://issues.apache.org/jira/browse/ZOOKEEPER-1452 Diffs ----- src/contrib/zkpython/README 89d9998 src/contrib/zkpython/src/c/pyzk_docstrings.h d2c4d60 src/contrib/zkpython/src/c/zookeeper.c 85fa512 Diff: https://reviews.apache.org/r/4958/diff Testing ------- Thanks, Henry
        Hide
        Aravind Narayanan added a comment -

        Patch containing first round of comments on ReviewBoard.

        Show
        Aravind Narayanan added a comment - Patch containing first round of comments on ReviewBoard.
        Hide
        Hadoop QA added a comment -

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

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

        +1 tests included. The patch appears to include 5 new or modified tests.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1223//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/12549093/ZOOKEEPER-1452.patch against trunk revision 1391526. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1223//console This message is automatically generated.
        Hide
        Aravind Narayanan added a comment -

        New patch with changes from the reviewboard comments. Also rebased on top of trunk.

        Show
        Aravind Narayanan added a comment - New patch with changes from the reviewboard comments. Also rebased on top of trunk.
        Hide
        Hadoop QA added a comment -

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

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

        +1 tests included. The patch appears to include 5 new or modified tests.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1224//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/12549095/ZOOKEEPER-1452.patch against trunk revision 1391526. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1224//console This message is automatically generated.
        Hide
        Aravind Narayanan added a comment -

        New patch containing changes based on the review comments.

        Hopefully this one won't be rejected...

        Show
        Aravind Narayanan added a comment - New patch containing changes based on the review comments. Hopefully this one won't be rejected...
        Hide
        Hadoop QA added a comment -

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

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

        +1 tests included. The patch appears to include 5 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 (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/1225//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1225//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1225//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/12549100/ZOOKEEPER-1452.patch against trunk revision 1391526. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 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 (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/1225//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1225//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1225//console This message is automatically generated.
        Hide
        Michi Mutsuzaki added a comment -

        Henry, is this good to go?

        Show
        Michi Mutsuzaki added a comment - Henry, is this good to go?
        Hide
        Hadoop QA added a comment -

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

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

        +1 tests included. The patch appears to include 5 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 (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/2034//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2034//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2034//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/12549100/ZOOKEEPER-1452.patch against trunk revision 1586200. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 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 (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/2034//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2034//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2034//console This message is automatically generated.

          People

          • Assignee:
            Aravind Narayanan
            Reporter:
            Aravind Narayanan
          • Votes:
            1 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:

              Time Tracking

              Estimated:
              Original Estimate - 336h
              336h
              Remaining:
              Remaining Estimate - 336h
              336h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development