ZOOKEEPER-1416 - Persistent, recursive watchers

[Randgalt]

Background

Note: this is a port of #136

Implementation for a persistent, recursive watch addition for ZK. These watches are set via a new method, addPersistentWatch() and are removed via the existing watcher removal methods. Persistent, recursive watches have these characteristics: a) Once set, they do not auto-remove when triggered; b) they trigger for all event types (child, data, etc.) on the node they are registered for and any child znode recursively; c) they are efficiently implemented by using the existing watch internals. A new class PathIterator walks up the path parent-by-parent when checking if a watcher applies.

Implementation Details

• A new enum manages the different "modes" for watchers: WatcherMode.
• For traditional, "standard" watchers, the code path is almost exactly the same. There is very little overhead other than a bit of extra checks in WatchManager
• Given how this is implemented it was difficult to add support when WatchManagerOptimized is used. I'm open to adding it for that version but it will take work. We should consider not supporting persistent/recursive watchers when WatchManagerOptimized is used. I notice that WatchManagerOptimized is not even mentioned in the docs.
• The mode for a given watcher/path pair is held in a map inside of WatcherModeManager. The absence of an entry means Standard. This way, there's no overhead for old, standard watchers.
• PathParentIterator is the "meat" of the implementation. Rather than set watchers on every ZNode implied by a recursive watcher. WatchManager passes any paths it processes through PathParentIterator which iterates up each parent znode looking for watchers.
• The remainder of the changes are scaffolding to match how other watchers are used as well as Jute/API changes to set persistent/recursive watchers

Testing

The tests were written years ago. I think they're comprehensive but reviewers should pay attention to anything that was missed. There is much ZooKeeper knowledge that's only in the heads of ZK committers.

• PersistentWatcherTest - tests persistent, non-recursive watchers
• PersistentRecursiveWatcherTest - tests persistent, recursive watchers
• PathParentIteratorTest- exercises edges of PathParentIterator

[eolivelli]

What about using an enum instead of a boolean for 'recursive'?

[Randgalt]

Do you anticipate more types? It would be easy to do.

[eolivelli]

Yes.
We could also call the method 'addWatch' and have enum values like PERSISTENT_RECURSIVE and PERSISTENT_RECURSIVE

Or even better we can use EnumSet and two flags PERSISTENT,RECURSIVE

addWatch(...., EnumSet.of(PERSISTENT,RECURSIVE))

[Randgalt]

The problem with an EnumSet (or a var args) is that it would allow empty sets/arrays which is illegal. I'd rather have a single enum argument.

[Randgalt]

So, I propose adding:

public enum PersistentWatchMode {
    NON_RECURSIVE,
    RECURSIVE
}

And changing the APIs to take this instead of the boolean. OK?

[Randgalt]

Note: this is actually a pretty big change. I'm going to flow the watch mode through the server and not just make this a client change.

[eolivelli]

we should narrow down the patch to the minimum needed, otherwise it will be hard to merge it soon.
if we have more complex plan for the future and change deeply protocols and server side flows we should discuss on the ML more.

[Randgalt]

So, you're suggesting a client only change? i.e. this enum is merely to avoid using a boolean in the public API?

[eolivelli]

yes
You already did a great work and the client with your changes is still compatibile at 100% with 3.5 servers in case of not using persistent watches.
If we change the protocol regarding watches (adding some watchtype to the wireprotocol) you will really need a "compatibility flag" on the client configuration or some more complicated handshake with version/feature discovery.

[Randgalt]

Look at the commit I just pushed. This is merely a refinement. I think this is much better actually and we'll thank ourselves in the future. Now, things are more generic "add watch with a mode". The mode is currently only persistent or recursive but we now have room to change in the future. The number/scope of the change is exactly the same as before.

[Randgalt]

FYI - I wrote the Curator portions of this new feature as a test. One of them is a new path cache that replaces Curator's TreeCache, PathChildrenCache and NodeCache. First, the code is so much simpler and easier to reason about. This alone will reduce errors and complexity. Also the number of ZK calls needed (and overhead for all those watchers) is vastly reduced.

I wrote a test to exercise the new cache, include with forced server crashes, and it's working perfectly. So, this suggest this new Watcher code is good. So, FYI, another data point.

If anyone's interested - here's the code:

• PersistentWatcher.java - keeps a persistent watcher registered past any connection losses
• CuratorCacheImpl.java - a full tree cache - note how simple it is compared to the corresponding Curator recipes that have to use old style watchers.

[Randgalt]

How can we get this completed?

[eolivelli]

Looks great to me.

Just waiting for approval from other committers.

Thank you so much for contributing this great feature

[Randgalt]

An important question to all committers. In DataTree.setWatches persistent watchers are not applied. This means that after a network partition, no persistent watchers will trigger. I don't have a feeling about this one way or another - the current implementation works fine for Curator's use cases.

[eolivelli]

@Randgalt can you elaborate a bit more?
What's the relationship between DataTree and network partition?

[tisonkun]

@eolivelli it is about SetWatches message which on connection loss recovered resets watchers on server and trigger if it detected event. In this pull request it reset persistent watches but no logic detect if there has been an event.

[Randgalt]

Yes. It’s the triggering of missed events that I’m concerned about. It would be easy to add but I’m unsure if it’s needed.

Note: it could trigger the entire tree of nodes. I’m very uncertain if it’s a good idea. I was able to write Curator’s new caching recipe without this.

Update: 1 possibility is to leave the implementation as it is and document the behavior.

[anmolnar]

@Randgalt I don't have a strong opinion either, but in case of a lot of missed events triggering them would potentially put very high load on the client which might not be desirable.

[anmolnar]

At a very quick glance it looks good to me. Here're a few points to think about:

• More detailed documentation would be nice to have. I understand the motivation behind having a completely separate API for these new watchers, but it seems to me it has a very important difference in its characteristics: standard watchers are set via getData() API in order to avoid losing any data change event. Retrieving the data and setting the watcher is atomic. Persistent watchers work differently.

• As you outlined we don't have docs for WatchManagerOptimized, but we do have a nice javadoc. Would be nice to add it here or create new Jira to fill the gap.

• CLI support - new Jira?

• C client support - new Jira?

• Add watcher type to Admin server: currently I can see persistent watcher, but there's no way to distinguish them from standard watchers. 4LW is deprecated, so we probably don't need support for it.

• WatchManagerOptimized: I haven't tried, but I think you can add the loop of PathIterator as for WatchManager. This guy here:

Watcher w = watcherBitIdMap.get(wBit);

retrieves the watcher and you can get the WatcherMode in the usual way. Maybe the hard part is that WatchManagerOptimized immediately removes the watchers here:

BitHashSet watchers = remove(path);

which could be not straightforward to refactor.
...or I completely misunderstand the problem.

[anmolnar]

Why not using ConcurrentHashMap instead of synchronizing everywhere?

[Randgalt]

I agree - but it should probably be a separate tech-debt ticket.

[anmolnar]

One more thing on the testing side.

Currently we run the same unit tests for all impl of WatchManagers in WatchManagerTest. If we end up not implementing persistent watcher for the optimized version, I think we will need to break these tests and separately test persistent/recursive watchers in the tests of standard WatchManager.

[Randgalt]

@Randgalt I don't have a strong opinion either, but in case of a lot of missed events triggering them would potentially put very high load on the client which might not be desirable.

The Curator cache recipes have to re-query the entire tree anyway so it doesn't benefit algorithms like that. So, my vote would be to document the behavior and let clients choose how to handle re-connections. But, I'm happy to change if others prefer something else.

[eolivelli]

I did not take into account the OptimizedWatchManager case.
I left a new comment, please take a look

[eolivelli]

This path will be followed with OptimizedWatchManager.
How will it bubble up to the client?

I see two ways:

• implement the method everywhere (it will have a cost!)
• add a test case about the behaviour of a client that it is trying to use this feature with a server that does not support it because it has not the default WatchManager impl

Thoughts?

[Randgalt]

@eolivelli test added. See: https://github.com/Randgalt/zookeeper/blob/ZOOKEEPER-1416/zookeeper-server/src/test/java/org/apache/zookeeper/test/UnsupportedAddWatcherTest.java

[Randgalt]

@anmolnar

• Re More detailed documentation would be nice to have - I'll push more docs soon
• Re the CLI - the CLI does not support watchers so it shouldn't be a concern
• Re C client support - A separate ticket. I don't know much about the C client.
• Re Add watcher type to Admin server - I'll see what I can do

@anmolnar and @eolivelli

• Re WatchManagerOptimized - I'll make sure tests pass, etc. without adding support to it. As @anmolnar noticed it will be very tricky to do.

---

@anmolnar I looked at the AdminServer and it currently doesn't return any details about watchers - i.e. whether it's a data watcher, exists watcher or a child watcher. In my view, nothing needs to be done to stay consistent.

[maoling]

• One of my concerns is:When the children of one path is very frequently updated flying with persistent-recursive watcher, the fired watches can be a issue?(e.g overhead network bandwidth. I test a path node with 1000000 children paths, then update all these 100000 child-paths with 10 threads, the watch works well.
• I saw this PR written two year ago. Thanks @Randgalt for unremitting efforts and awesome work.

[eolivelli]

good, we need a final +1 from at least @anmolnar then we can go and release this feature in 3.6.0

[Randgalt]

• I saw this PR written two year ago.

Actually, 3 years ago 😃

[eribeiro]

@Randgalt Congratulations for the awesome work!!! 🎉 It's very nice to see this much anticipated feature being finally merged into trunk. 👏

[Randgalt]

@Randgalt Congratulations for the awesome work!!! 🎉 It's very nice to see this much anticipated feature being finally merged into trunk. 👏

It's been 3 years. I'll believe it when it actually happens 😄

[nkalmar]

+1
builds, test run, tested with zkCli, command works, mode switch works, new watch works as expected.

[eolivelli]

@anmolnar shall I have the honour of merging this patch ?

[anmolnar]

@eolivelli +1 Go ahead please.

[tisonkun]

Yes. It’s the triggering of missed events that I’m concerned about. It would be easy to add but I’m unsure if it’s needed.

@Randgalt I run into this case today. How can it be easy to add? At least we need to associate zxid with the request and the data tree node, as well as bookkeep the changes. Then we can filter the changes between the client setwatches request and the server state. Or we send some duplicate but the client can filter with some revisions.

[Randgalt]

Yes. It’s the triggering of missed events that I’m concerned about. It would be easy to add but I’m unsure if it’s needed.

@Randgalt I run into this case today. How can it be easy to add? At least we need to associate zxid with the request and the data tree node, as well as bookkeep the changes. Then we can filter the changes between the client setwatches request and the server state. Or we send some duplicate but the client can filter with some revisions.

The persistentWatches should be easy. Something like:

        for (String path : persistentWatches) {
            DataNode node = getNode(path);
            // determine if there's a change similar to code above this and process the watch if needed
            this.childWatches.addWatch(path, watcher, WatcherMode.PERSISTENT);
            this.dataWatches.addWatch(path, watcher, WatcherMode.PERSISTENT);
        }

persistentRecursiveWatches are harder. For each persistent recursive watch you'd need to walk down child paths of each watch, get the node and determine if the watch needs to be called (you'd probably need to keep a set of already visited nodes - I'm not sure). Depending on the level of the watch it could end up walking the entire database but there's no choice.

[tisonkun]

@Randgalt I get your point.

similar to code above this and process the watch if needed

Although, this can be more trick than it first looks like. Because a persistent watch is always set, only node.stat.getMzxid() > relativeZxid indicates a NodeDataChanged event (It can be actually a NodeCreated, which we cannot distinguish). Either the node exists or not cannot indicate NodeCreated or NodeDeleted. Also, at most it resends the latest event, instead of all the events during the network partition.

But I think I know what we can do and your original thought now :)