[
https://issues.apache.org/jira/browse/ZOOKEEPER-965?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13032669#comment-13032669
]
Marshall McMullen commented on ZOOKEEPER-965:
---------------------------------------------
Stephen,
Thanks very much for doing a thorough code review of the C library. It is super
appreciated. I think all your points are valid. Specifically, comments in-line
below:
* The zoo_acheck comment includes documentation on a member 'stat' that is not
an argument
* The zoo_check comment has the parameter stat listed as _stat
Copy-paste errors :-). Fixed.
* In create_completion_entry, you commented out the else case where you set
c->clist to NULL. Did you mean to leave this in there?
I had at one point had that field as a pointer instead of a struct. Instead I
should probably zero out that structure to be safest. It shouldn't matter in
this case, but it's better to be overly cautious I think.
* In op_result_string_completion, you memcpy from the value onto data->value
based on the size of value. Is data->value guaranteed to be larger than value?
No, that's a very good catch. I need to take the length into account and deal
with that error path accordingly as we do elsewhere. I'll check in a fix for
this.
* You only assert result in op_result_string_completion, but none of the other
op_result_* functions
Nice catch. I'll add that to my fixes.
* Do you not need to set result->err in op_result_multi_completion?
No, I don't think so. Unlike the other op_result_* completions, this one
doesn't take a op_result_t that I fill in the return code. This purpose of this
completion is to hold the list of completions for each op in the multi op. That
way when we see a multi op, we can pop off the completions and call them. You
may have noticed I append a COMPLETION_VOID (I called this the "tail
completion" at the end of the multi's completion list. This is a void
completion that *does* set the return value in the completion so it propagates
to the caller. Does that make sense? I'll add some comments to the completion
to explain.
* In zoo_amulti, any reason "int index = 0;" isn't a few lines down right
before it's used?
Nope, probably result of moving stuff around a lot. I'll clean that up.
* Slick zoo_amulti code, very nicely done.
:-] THANKS!! I wrestled with the implementation for a long time. I'm happy with
how it came out.
* In testCreate you compare results[*].err to 0, which will work, but shouldn't
you compare it to ZOK? Or do I misunderstand op_result_t.
Yes, you're right. I also should reverse those arguments to
CPPUNIT_ASSERT_EQUAL as we always have the expected followed by the actual. I
also found that error elsewhere in that test file that I fixed.
* In testDelete, your comment should say "// '/multi2' should have been deleted"
Yep, good catch.
Again, thanks for all the valuable and appreciate feedback. I'll commit these
changes shortly.
> Need a multi-update command to allow multiple znodes to be updated safely
> -------------------------------------------------------------------------
>
> Key: ZOOKEEPER-965
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-965
> Project: ZooKeeper
> Issue Type: Bug
> Affects Versions: 3.3.3
> Reporter: Ted Dunning
> Assignee: Ted Dunning
> Fix For: 3.4.0
>
> Attachments: ZOOKEEPER-965.patch, ZOOKEEPER-965.patch,
> ZOOKEEPER-965.patch, ZOOKEEPER-965.patch, ZOOKEEPER-965.patch,
> ZOOKEEPER-965.patch, ZOOKEEPER-965.patch, ZOOKEEPER-965.patch,
> ZOOKEEPER-965.patch, ZOOKEEPER-965.patch
>
>
> The basic idea is to have a single method called "multi" that will accept a
> list of create, delete, update or check objects each of which has a desired
> version or file state in the case of create. If all of the version and
> existence constraints can be satisfied, then all updates will be done
> atomically.
> Two API styles have been suggested. One has a list as above and the other
> style has a "Transaction" that allows builder-like methods to build a set of
> updates and a commit method to finalize the transaction. This can trivially
> be reduced to the first kind of API so the list based API style should be
> considered the primitive and the builder style should be implemented as
> syntactic sugar.
> The total size of all the data in all updates and creates in a single
> transaction should be limited to 1MB.
> Implementation-wise this capability can be done using standard ZK internals.
> The changes include:
> - update to ZK clients to all the new call
> - additional wire level request
> - on the server, in the code that converts transactions to idempotent form,
> the code should be slightly extended to convert a list of operations to
> idempotent form.
> - on the client, a down-rev server that rejects the multi-update should be
> detected gracefully and an informative exception should be thrown.
> To facilitate shared development, I have established a github repository at
> https://github.com/tdunning/zookeeper and am happy to extend committer
> status to anyone who agrees to donate their code back to Apache. The final
> patch will be attached to this bug as normal.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
