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