ZooKeeper

Committed revision 174.

Excellent work Pat! Thanx.

this patch (v4) addresses all of the outstanding issues that were
identified and also fixes server bug 1981340.

please review and let me know your feedback.

Note:
I added a bunch of new watcher tests for all of the various sync read
operations (exist/getdata/getchildren) for both the legacy and watcher obj
registration functions. these tests also verify 1981340.

as part of adding the tests I refactored clienttest a bit (extract base).

note that I commented out the "sleep(5000)" in the test setup of
clientbase. is there a reason why this was there? (works fine for me on all
the tests w/o this line). the tests run a whole lot faster if this is
commented out - if it needs to be there feel free to add it back. too bad
we can't wait() on the the server object... perhaps we should fix this at
some point (server does notifyall on state change).


File Added: watcherobj4.patch

Looks good! You are still missing 3 and 4.

3) Since there is currently only one watch, yes all watches are notified
of a disconnect.

4) Yes, this is a bug in the server. The current behaviors leave
untriggered watches which is not right. I've opened a bug for this: [
1981340 ] Child watches are not triggered when the node is deleted

I implemented all of your (ben) suggestions except for item 3. This keeps
the behavior as close to the original as possible - the default watcher
will be called when not sync'd.

I register the watcher in finish now - at the very beginning of the
function. Please verify this in particular is correct.

File Added: watcherobj3.patch

regarding:

4) as part of reworking the patch I looked closely at the code for
triggering watches and based processwatchevent on that, unless I'm missing
something what I have is correct behavior -- here is deletenode code on the
server:

dataWatches.triggerWatch(path, Event.EventNodeDeleted);
childWatches.triggerWatch(parentName.equals("")?"/":parentName,
Event.EventNodeChildrenChanged);

childwatches get childrenchanged on the parent of path while datawatches
gets nodedeleted on the path itself

2) so the assumption is that two sets of code won't register the same
watch obj? This seems reasonable, but I will make explicit in the docs

3) is that what happens currently? all watches are triggered on
disconnect?

Otw I'll incorporate the feedback you provided.

This looks much better. There are still a couple of issues:

1) The XXXWatches hashmaps should be final not volatile
2) The elements of the watch hashmaps should be hashsets, not lists. If
the same watch is added twice, we only need to invoke once.
3) In processWatchEvent, on disconnect I think all the watches need to be
invoked not cleared. This may not really be an issue since I'll be changing
this with the auto reregister of watches.
4) In processWatchEvent, the nodeDeleted event gets delivered to both data
and child watchers. (If you are watching for children of a node, and that
node goes away we trigger a watch.)
5) In processWatchEvent, the remove from the hashmap needs to be done in a
synchronized block. Otherwise, you could have concurrent additions to the
Hashset, currently ArrayList, going on.
6) You cannot add the watches in the completion function and the blocks of
the synchronous calls. If you do you end up with a race condition: you are
interested in a change, it comes in right after you get the response but
before you actually process it. They all need to be done in one place:
finishPacket(); the next packet will not be processed before finishPacket
is complete.

I updated the patch based on feedback on original.
File Added: watcherobj2.patch

As far as the implementation goes, we can't do it in the clientcnxn
EventThread since only async events go there. The easiest thing would be to
do it in finishPacket(). You should probably add members in Packet to hold
the Watcher object put it in watches in finishPacket().

'Are you saying that if I call getData(node,true) and exists(node,true)
and
then a subsequent create(node) happens I will get a single watch event of
type datachanged for node? I won't see a "datachanged(node)" as well as a
"nodecreated(node)", one for each of the read calls respectively?'

If a create(node) succeeds, that means the node did not exists, which
means that getData got a NoNode error, which means that no watch was set.
So, you will only see a nodecreated event and it would be delivered to the
Watcher of the exists() call.

What would you suggest regarding async read requests and watcher
registration - we wrap the callback with code that registers the watcher
then calls the client callback? I think this would be cleaner than
embedding into the clientcnxn EventThread code. What do you think?

Hm. I re-read the docs and I'm still a bit confused by your statements
regarding watch types.

Are you saying that if I call getData(node,true) and exists(node,true) and
then a subsequent create(node) happens I will get a single watch event of
type datachanged for node? I won't see a "datachanged(node)" as well as a
"nodecreated(node)", one for each of the read calls respectively?

The watches wiki http://zookeeper.wiki.sourceforge.net/ZooKeeperWatches
should really be beefed up with more detail on which event types are
generated based on write ops.

I'll rework the patch. Regarding the c vs java - I only implemented java
due to the category being set to "java client". Perhaps we should add a
"client API" category? Or maybe create two requests instead of 1 -- would
be better for me since I'm much more familiar with java than C, you could
assign to someone better able to execute. :-)

We do also need to implement this in the C client as well, but for
simplicity we should make that a separate patch.

We shouldn't add watches to the watches hashmap until we get a successful
result back from the server. Otherwise, we might trigger watches for things
that the client isn't expecting.

There is also a problem with type watch types. In the server we have a
watch list for data and child watches. It might be good to keep that same
division in the client. (Either way would work though.)

a watch on getChildren() can result in a childrenChanged or nodeDeleted
watch event.
getData() can result in dataChanged or nodeDeleted event.
exist() can result in dataChanged, nodeCreated, or nodeDeleted event.

getChildren() and getData() will not get nodeCreated events, because non
existence is an error condition for those two calls and a watch will not be
set.

Here's the patch, assigning to Ben for further review.

Please pay particular attention to the handling of state changes - I clear
the watch table when we are not sync'd to the server. Verify my workflow.

File Added: watcherobj.patch

I've implemented this for Java. I overloaded the respective methods with
signatures that take watcher object instead of watch boolean. I added new
methods rather than replacing -- this maintains b/w compatibility.

1) should I mark the old methods as deprecated or should we just support
both method types (newer signature w/ explicit watcher object && old watch
boolean which directs to the "global" watcher.)

2) I see this is listed as category "java client" so I'm assuming there is
no need for this feature in c code interface.

I was specifically referring to the client api, not the protocol -- should
I assume that that last watcher on a particular node "wins". What I mean is
if a client calls "getChildren(path, watcherobj1); getChildren(path,
watcherobj2);" (notice same patch) what should the client lib do? I'm
assuming use the last watcher, but wanted to verify.

The client will need to track the multiple watcher objects to invoke when
it gets the callback. The client server protocol doesn't change, this is
just a client side thing. We should add the ability to remove watches as a
separate feature request.

What should we do if the user calls getData multiple times with different
watcher objects? Remove previous watch and replace with new watcher, throw
error, or allow multiple watchers on particular node? How about removing a
watch by calling method on particular node with null watcher?