CURATOR-344 - limit shared value watcher to valid change events to av…

[gtully]

…oid work on disconnect.

Test that shows the difficulty with blocking the event thread.
When the framework is shared and one use case needs timely notification of suspend, a use case that requires retry for some period gets in the way.

In any event, on a disconnect, there may or may not be change, it does not make sense to me that the listeners should be fired.

[cammckenzie]

I don't think that this completely fixes the problem. The test fails intermittently because it's possible that the server gets stopped while the SharedValue is processing the initial connected event. When this happens, it goes through the retry loop and the assertion at the end of the test fails.

I think that ultimately the processing that occurs within the watcher's process() method should be run asynchronously, and equally, the watcher should be more discriminant about which events it's interested in.

Thoughts?

[gtully]

Thanks for taking the time to peek at this. I agree.
Doing everything async will make it a little heaver weight but I think it is more correct.
There seems to be be some recipes that take executors, others that don't, but I have not figured out if there is some pattern to this. Do you know?

Short term, I can fix up the unit test to be deterministic (i.e. wait for connected).
Maybe this can be tackled in two phases. The first making the watcher more discriminating. For my use case, disconnecting is the important event.

[cammckenzie]

I'm not sure on the pattern for executors, probably just the whim of whoever implemented the recipe I imagine.

I think that you should be able to do this asynchronously without an executor though. If you change the readValue() method to have an option of executing in the background then it should fix the problem I think. When being called from the Watcher, you would make the readValue getData() call in the background and update the shared value in the asynchronous callback.

That should fix the problem properly and not require the introduction of an executor. I think that we should clean up the events which actually result in the readValue() being called from the Watcher too.

[gtully]

thanks for the feedback.
I fixed up the unit test to make it more deterministic.
Changed the watcher to do an update and notification in the background.
That sorts my use case however the update events should only fire on valid change events.
Added additional unit test to track change events between disconnect and reconnect and made the matching change to restrict the callback to valid change events.
I think it is better on both counts.

[cammckenzie]

Looks good to me, thanks for the patch!