CURATOR-727: Allow watches to be executed asynchronously #514
Conversation
| } | ||
| }; | ||
| } | ||
| if (watchRunnable != null) { |
There was a problem hiding this comment.
Perhaps more elegant to add an "else" that returns. Therefore, watchRunnable could be final non-null; no condition to act on it's null-ness.
| @@ -445,6 +447,18 @@ public void close() { | |||
| unhandledErrorListeners.clear(); | |||
| connectionStateManager.close(); | |||
| client.close(); | |||
|
|
|||
| if (asyncWatchService != null) { | |||
There was a problem hiding this comment.
If one of those long-running processes interacts with curator, I could see it failing. It'd be better to have close() relatively early call shutdown(), after publishing the close event.
There was a problem hiding this comment.
after publishing the close event.
That would happen either after connectionStateManager.close() or client.close(). So this is happening just after those are called.
But I do think we would want to skip any client connection management watches here. Those probably shouldn't be asynchronous anyways. So I'm going to look into that.
There was a problem hiding this comment.
If the "asyncExecutor" could in fact always be pre-initialized to a "direct" (same thread Executor like Guava MoreExecutors.newDirectExecutorService), then there'd be no branching check around if it's present or not. Solr uses this technique in SimpleFacets too. That said, there's little complexity here so I'm not going to recommend it.
|
cc @eolivelli @kezhuw @Randgalt @cammckenzie Not quite sure if such a change would break sequential assumption. |
|
Hi @HoustonPutman! We just released Curator 5.8.0 and it causes several code conflict. To move this PR forward, I'd suggest you rebase this patch or open a new one, referring to #1242. And the key blocker here is to analyze and describe how running watches in a dedicated executor service won't break current sequential implementation. That is, do we depend on the sequential assumption? Or describing the current execution order to ensure that we won't break any assumption. |
|
Thanks for the update @tisonkun. Yeah, there is a lot to think about there, and probably will require a more intelligent feature. This isn't a blocker for us, so we can move forward as-is with this release in Solr. But as we transfer leader election and other big features to recipes, we can come back and rethink how to approach the watch executor. |
https://issues.apache.org/jira/browse/CURATOR-727
This gives users the ability to run all watches asynchronously, via a provided executorService.