[
https://issues.apache.org/jira/browse/ZOOKEEPER-1452?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13266227#comment-13266227
]
[email protected] commented on ZOOKEEPER-1452:
----------------------------------------------------------
-----------------------------------------------------------
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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4958/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-05-01 22:22:51)
bq.
bq.
bq. Review request for zookeeper and Henry Robinson.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Creating this review request on behalf of Aravind Narayanan for
ZOOKEEPER-1452
bq.
bq.
bq. This addresses bug ZOOKEEPER-1452.
bq. https://issues.apache.org/jira/browse/ZOOKEEPER-1452
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. src/contrib/zkpython/README 89d9998
bq. src/contrib/zkpython/src/c/pyzk_docstrings.h d2c4d60
bq. src/contrib/zkpython/src/c/zookeeper.c 85fa512
bq.
bq. Diff: https://reviews.apache.org/r/4958/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Henry
bq.
bq.
> zoo_multi() & zoo_amulti() update operations for zkpython
> ---------------------------------------------------------
>
> Key: ZOOKEEPER-1452
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-1452
> Project: ZooKeeper
> Issue Type: Improvement
> Components: contrib-bindings
> Reporter: Aravind Narayanan
> Assignee: Aravind Narayanan
> Priority: Critical
> Labels: python
> Fix For: 3.5.0
>
> Attachments: ZOOKEEPER-1452.patch
>
> Original Estimate: 336h
> Remaining Estimate: 336h
>
> 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.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
