Review Board 1.7.22


zoo_multi() & zoo_amulti() update operations for zkpython

Review Request #4958 - Created May 1, 2012 and updated

Henry Robinson
ZOOKEEPER-1452
Reviewers
zookeeper
Henry
zookeeper-git
Creating this review request on behalf of Aravind Narayanan for ZOOKEEPER-1452

 
Total:
20
Open:
20
Resolved:
0
Dropped:
0
Status:
From:
Description From Last Updated Status
Done. Aravind Narayanan Oct. 14, 2012, 8:42 p.m. Open
Done. Aravind Narayanan Oct. 14, 2012, 8:42 p.m. Open
Done. Aravind Narayanan Oct. 14, 2012, 8:42 p.m. Open
Done Aravind Narayanan Oct. 14, 2012, 8:42 p.m. Open
Done. Aravind Narayanan Oct. 14, 2012, 8:42 p.m. Open
Done. Aravind Narayanan Oct. 14, 2012, 8:42 p.m. Open
Done. Aravind Narayanan Oct. 14, 2012, 8:42 p.m. Open
Done. Aravind Narayanan Oct. 14, 2012, 8:42 p.m. Open
Done. Aravind Narayanan Oct. 14, 2012, 8:42 p.m. Open
Done. Aravind Narayanan Oct. 14, 2012, 8:42 p.m. Open
Done Aravind Narayanan Oct. 14, 2012, 8:42 p.m. Open
Done. Aravind Narayanan Oct. 14, 2012, 8:42 p.m. Open
Done. Checking PyErr_Occurred now. Aravind Narayanan Oct. 14, 2012, 8:42 p.m. Open
Oh, right, done. Aravind Narayanan Oct. 14, 2012, 8:42 p.m. Open
Done. Aravind Narayanan Oct. 14, 2012, 8:42 p.m. Open
Done. Aravind Narayanan Oct. 14, 2012, 8:42 p.m. Open
Done. Aravind Narayanan Oct. 14, 2012, 8:42 p.m. Open
Done. Also fixed the switch in `serialize_multi_ops()`. Aravind Narayanan Oct. 14, 2012, 8:42 p.m. Open
Done. Aravind Narayanan Oct. 14, 2012, 8:42 p.m. Open
Fixed. I'm starting to see a pattern here :) Aravind Narayanan Oct. 14, 2012, 8:42 p.m. Open
Review request changed
Updated (May 1, 2012, 10:22 p.m.)
Woops, wrong diff. 
Posted (May 1, 2012, 11:04 p.m.)
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.
Typo: synchronously
Can you expand on this a bit - what kind of data structure should an 'op' be represented as in Python?
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 (Diff revision 2)
 
 
Nit: char *, not char*
src/contrib/zkpython/src/c/zookeeper.c (Diff revision 2)
 
 
nit: Stat *stat
src/contrib/zkpython/src/c/zookeeper.c (Diff revision 2)
 
 
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 (Diff revision 2)
 
 
Nit: I think this should be PyExc_TypeError
src/contrib/zkpython/src/c/zookeeper.c (Diff revision 2)
 
 
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 (Diff revision 2)
 
 
If the serialize fails, do you need to free buf?
src/contrib/zkpython/src/c/zookeeper.c (Diff revision 2)
 
 
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 (Diff revision 2)
 
 
typo: amulti
src/contrib/zkpython/src/c/zookeeper.c (Diff revision 2)
 
 
Do you need to call PyGILState_Release here? What about Py_DECREF(pyresults)?
src/contrib/zkpython/src/c/zookeeper.c (Diff revision 2)
 
 
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 (Diff revision 2)
 
 
"Completion parameter must be callable"
src/contrib/zkpython/src/c/zookeeper.c (Diff revision 2)
 
 
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)
Posted (Oct. 14, 2012, 8:42 p.m.)

   

  
Done.
Done.
Done.
Done
src/contrib/zkpython/src/c/zookeeper.c (Diff revision 2)
 
 
Done.
src/contrib/zkpython/src/c/zookeeper.c (Diff revision 2)
 
 
Done.
src/contrib/zkpython/src/c/zookeeper.c (Diff revision 2)
 
 
Done.
src/contrib/zkpython/src/c/zookeeper.c (Diff revision 2)
 
 
Done.
src/contrib/zkpython/src/c/zookeeper.c (Diff revision 2)
 
 
Done.
src/contrib/zkpython/src/c/zookeeper.c (Diff revision 2)
 
 
Done.
src/contrib/zkpython/src/c/zookeeper.c (Diff revision 2)
 
 
Done
src/contrib/zkpython/src/c/zookeeper.c (Diff revision 2)
 
 
Done.
src/contrib/zkpython/src/c/zookeeper.c (Diff revision 2)
 
 
Done. Checking PyErr_Occurred now.
src/contrib/zkpython/src/c/zookeeper.c (Diff revision 2)
 
 
Oh, right, done.
src/contrib/zkpython/src/c/zookeeper.c (Diff revision 2)
 
 
Done.
src/contrib/zkpython/src/c/zookeeper.c (Diff revision 2)
 
 
Done.
src/contrib/zkpython/src/c/zookeeper.c (Diff revision 2)
 
 
Done.
src/contrib/zkpython/src/c/zookeeper.c (Diff revision 2)
 
 
Done. Also fixed the switch in `serialize_multi_ops()`.
src/contrib/zkpython/src/c/zookeeper.c (Diff revision 2)
 
 
Done.
src/contrib/zkpython/src/c/zookeeper.c (Diff revision 2)
 
 
Fixed. I'm starting to see a pattern here :)