[CURATOR-549] Recipes based on Persistent Recursive Watchers

[Randgalt]

See: apache/zookeeper#1106

This PR adds several recipes that use the new ZOOKEEPER-1416 Persistent Recursive watches from ZooKeeper 3.6.0.

PersistentWatcher

A wrapper recipe that keeps a persistent (single or recursive) watch set and active through disconnections, etc.

See: PersistentWatcher.java

CuratorCache

Completely re-written cache recipe that will replace TreeCache, NodeCache and PathChildrenCache. With the benefit of persistent recursive watchers and the new PersistentWatcher recipe, the implementation is far simpler, will use significantly less resources and network calls, be easier to support and should be more stable and performant.

Wrappers for the older cache's listeners are provided to help with transitions.

See:

• CuratorCache - this is the API
• CuratorCacheImpl - this is the implementation

[tisonkun]

Thanks for your work! I'm reviewing now.

[tisonkun]

Beautiful work! Only minor comments.

[tisonkun]

Is this method for compatibility of PathChildrenCache#getCurrentData? If so, shall we mention it?

[Randgalt]

I'm not happy with this API. I'm going to change it. Please refer to the next commit.

[tisonkun]

Shall we add a test or example for the use case? I don't see the motivation here in practice.

[Randgalt]

@tisonkun 2 comments have been addressed.

[eolivelli]

What not final?

[Randgalt]

It gets set to null after completionProc is called to signal decrement/increment are no longer necessary.

[eolivelli]

I am not sure this trick works.

You can be here when another thread calls increment function.
You are not inside a lock

[Randgalt]

Look at the next line if ( localCount.compareAndSet(0, Long.MIN_VALUE) ) - that ensures consistency (please review and confirm).

[Randgalt]

Hmm - I think there's a race here - I'm going to address.

[Randgalt]

@eolivelli I reworked it - please re-check

[tisonkun]

An alternative is that java.util.concurrent.Phaser which already implements semantic like OutstandingOps.

Runnable r = () -> callListeners(CuratorCacheListener::initialized);
Phaser p = new Phaser() {
  @Override
            protected boolean onAdvance(int phase, int registeredParties) {
                r.run();
                return true;
            }
}

and replace increment as register while decrement as arriveAndDeregister

[Randgalt]

Ooh - TIL - I didn't know about that. Thanks. Looking into it now.

[Randgalt]

Phaser looks to be a kind of barrier. How would it be used in this context @tisonkun ?

[tisonkun]

It seems OutstandingOps is a one-shot barrier, so that as the code snippet above when we have new outstanding op we register a party and terminate the phaser at the first time it is advanced. Is it the semantic of OutstandingOps?

[Randgalt]

It's the same idea as in TreeCache. When the cache is started each outstanding operation increments a counter and when the op completes it decrements the counter. It's not exact, but it provides a way to handle the "cache is initialized" feature. Once the outstanding ops counter gets back to 0 it's no longer used.

[tisonkun]

Yes that is the same as things in my mind. Since OutstandingOps implemented correctly now, we can have a follow-up to see if it's worth we replace it with Java builtins and I will share a patch with description there. At most it is an "improvement" and should not block this PR to merge :)

For the "how", it seems to me that #335 (comment) is clear.

[eolivelli]

The patch is so huge.
I don't feel I can give a real +1

I like this work.

I would suggest to create little patches for the future. this way reviews can be narrower.

[eolivelli]

Much better!

[shayshim]

This comment was never very clear to me, and if it is not only me, maybe it is good opportunity to rephrase it or give more details.
By "false positive" here do you mean that we might get events that do not reflect anymore the latest state of the zknode?
These events are still true, just a bit late. Soon there should follow the subsequent events until latest state is reflected. This behavior is general in ZK clients, not just the cache.
By "false negative" do you mean that we can miss events because ZK only sends the latest event (e.g. while cache might have been disconnected for a while, missing some zknode events)? Again, I feel like it is true for any ZK client, not just the cache. Though in Persistent Watchers I guess we potentially miss less momentarily events, because the Watcher is always there.

[shayshim]

Minor corner, but calling close() on cache that wasn't started is a no-op. Intuitively to me it should remain CLOSED in this case. If you agree, you could instead use:
if (state.getAndSet(CLOSED) == STARTED) ...

[Randgalt]

This pattern is used throughout Curator. It's actually an error to call close() without first calling start(). Maybe it should be an exception. We can deal with all instances of this pattern in a separate issue.

[shayshim]

If that's the pattern everyone already used to, then I think it is better to leave it without change. It is minor consideration anyways, and always can be overridden with a boolean paired to the cache from the outside.

[shayshim]

This call is going to also get all data of children recursively, and update the storage accordingly, so why do we need to pass the storage again as below?

[Randgalt]

Consider the case where the cache is in sync and then there is a network partition for a period of time. When the connection is restored, every node in the cache has to be re-considered as the new state of the server is not known. You can't just rebuild the cache from zero either as the client still needs to get events for changes. Neither can we just rebuild from the root node as checkChildrenChanged() stops if the cVersion is the same. E.g.

Pre network partition:

root
      Node A1
            Node B1
                Node C1
      Node A2

Post network partition:

root
      Node A1
            Node B1
                Node C2       --- C1 deleted, C2 is new
      Node A2

Rebuilding only from the Root would not detect that C1 was deleted and C2 was added because the cVersion of A1 would be the same.

[shayshim]

This cache does not allow to maintain only the immediate children? I noticed parentPathFilter, but shouldn't we allow that for efficiancy? (for cases of many grandchildren and so deeper that their updates are not interesting in this cache context.) Maybe not useful enough to bother, just asking.

[Randgalt]

If someone really needs to emulate PathChildrenCache it could be done with a custom implementation of CuratorCacheStorage. I thought it added too much weight to have APIs that only cache 1 level.

[Randgalt]

Also - the new CuratorCache uses a Persistent watcher internally (which I added to ZK for 3.6). Those watchers only have 2 modes - single node watch or entire tree watch. So, the new CuratorCache matches this.

[shayshim]

Yeah, it is kind of limitation of the Persistent Watcher itself - not be able to be Persistent only for immediate children of the node. Potentially for client that needs only immediate children, it can make both ZK server and client work harder for potentially many/large not-interesting sub-trees events. But maybe in real life it is not useful enough.

[shayshim]

General question about the inBackground callbacks - they are all going to be serialized on the single ZK event thread?

[Randgalt]

Yes, background callbacks are called from the single ZK event thread. Is there a concern?

[shayshim]

No concern, just wanted to make sure. addListener(T listener, Executor executor) can be used to offload heavy listeners to another single threaded Executor.

[shayshim]

In case of InterruptedException you both reset the interrupt on the thread and propagate the exception to user. Shouldn't you do only one of them, but not both?

[Randgalt]

It's important to reset the interrupted flag. I don't think we should count on the end user doing that. Curator has had this behavior for a long time. Maybe we can re-visit it in another issue.

[Randgalt]

The patch is so huge.
I don't feel I can give a real +1

I like this work.

I would suggest to create little patches for the future. this way reviews can be narrower.

This is already a broken up patch. There was a PR that preceded it. There will be another PR after this. So, this is as small as I could get this portion. Though, I guess I could have done just the PersistentWatcher class and then this.

[Randgalt]

@shayshim I reworked the doc for CuratorCache. Please re-review.

[shayshim]

Instead of having these multiple combinations (and the user will still need to check type for each event e.g. type == NODE_CHANGED) you could introduce interface that allows the user implement only the methods that are interesting for him/her, and have the burden of type -> action mapping specified in one location for all users:

public interface CacheListener { 
  void onCacheInitialized();
  void onNodeCreate(ChildData data);
  void onNodeChanged(ChildData oldData, ChildData data);
  void onNodeDeleted(ChildData data);
}

(you could introduce empty implementation for this interface for convenience).
It is approach that doesn't embrace lambdas, but in this case of multiple event types it might be more comfortable for users.
Note that you could do CuratorCacheListener -> this listener mapping as implementation of CuratorCacheListener (user gives you impl of this interface and you build around it CuratorCacheListener wrapper that does the mapping).

[Randgalt]

We could have both actually. We can add a CacheListenerAdaptor that looks like what you have. However, since Java 8, haven't most devs moved to the neo-functional style? The benefit of how I did the listener builder is that you don't have to use cumbersome anonymous classes. i.e. you can do:

CuratorCacheListener.builder()
    .forDeletes(node -> handleDelete(node))
    .forInitialized(() -> handleInitialized())
    ... etc ...

This is superior to anonymous inner classes.

[Randgalt]

The adaptor would look like this. Should I add this? Is it gilding the Lilly? https://gist.github.com/Randgalt/cbe38cb3cdf1825f73dd165d997a2248

[shayshim]

No, I take it back. Didn't understand fully your builder until now. We Don't need this interface and adapter.

[shayshim]

Great work!