You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@curator.apache.org by "HoustonPutman (via GitHub)" <gi...@apache.org> on 2024/12/16 17:17:51 UTC
[PR] CURATOR-727: Allow watches to be executed asynchronously [curator]
HoustonPutman opened a new pull request, #514:
URL: https://github.com/apache/curator/pull/514
https://issues.apache.org/jira/browse/CURATOR-727
This gives users the ability to run all watches asynchronously, via a provided executorService.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CURATOR-727: Allow watches to be executed asynchronously [curator]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #514:
URL: https://github.com/apache/curator/pull/514#issuecomment-2722395248
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CURATOR-727: Allow watches to be executed asynchronously [curator]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #514:
URL: https://github.com/apache/curator/pull/514#issuecomment-2705456883
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 https://github.com/apache/curator/issues/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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CURATOR-727: Allow watches to be executed asynchronously [curator]
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on code in PR #514:
URL: https://github.com/apache/curator/pull/514#discussion_r3630809513
##########
curator-framework/src/main/java/org/apache/curator/framework/CuratorFrameworkFactory.java:
##########
@@ -519,6 +521,19 @@ public Builder runSafeService(Executor runSafeService) {
return this;
}
+ /**
+ * By default, watches are run sequentially.
+ * If an executor is provided here, then all watch calls will be run asynchronously via this executor.
+ * This executor service will be closed when the CuratorFramework is closed.
+ *
+ * @param asyncWatchService executorService to use for all watch calls
+ * @return this
+ */
+ public Builder asyncWatchService(ExecutorService asyncWatchService) {
Review Comment:
I would probably name this method "executeWatchesAsync"
##########
curator-framework/src/main/java/org/apache/curator/framework/CuratorFrameworkFactory.java:
##########
@@ -519,6 +521,19 @@ public Builder runSafeService(Executor runSafeService) {
return this;
}
+ /**
+ * By default, watches are run sequentially.
+ * If an executor is provided here, then all watch calls will be run asynchronously via this executor.
+ * This executor service will be closed when the CuratorFramework is closed.
+ *
+ * @param asyncWatchService executorService to use for all watch calls
+ * @return this
+ */
+ public Builder asyncWatchService(ExecutorService asyncWatchService) {
Review Comment:
is there any additional constraint on the ExecutorService ?
can operations be executed in any order ?
what happens in case of rejected execution ?
what happens if the watcher code "throws" ? should the ExecutorService be resilient ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CURATOR-727: Allow watches to be executed asynchronously [curator]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #514:
URL: https://github.com/apache/curator/pull/514#discussion_r1887528639
##########
curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java:
##########
@@ -445,6 +447,18 @@ public void close() {
unhandledErrorListeners.clear();
connectionStateManager.close();
client.close();
+
+ if (asyncWatchService != null) {
Review 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.
##########
curator-framework/src/main/java/org/apache/curator/framework/imps/NamespaceWatcher.java:
##########
Review 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.
##########
curator-framework/src/main/java/org/apache/curator/framework/imps/NamespaceWatcher.java:
##########
@@ -65,14 +66,25 @@ public void process(WatchedEvent event) {
client.getWatcherRemovalManager().noteTriggeredWatcher(this);
}
+ Runnable watchRunnable = null;
if (actualWatcher != null) {
- actualWatcher.process(new NamespaceWatchedEvent(client, event));
+ watchRunnable = () -> actualWatcher.process(new NamespaceWatchedEvent(client, event));
} else if (curatorWatcher != null) {
- try {
- curatorWatcher.process(new NamespaceWatchedEvent(client, event));
- } catch (Exception e) {
- ThreadUtils.checkInterrupted(e);
- client.logError("Watcher exception", e);
+ watchRunnable = () -> {
+ try {
+ curatorWatcher.process(new NamespaceWatchedEvent(client, event));
+ } catch (Exception e) {
+ ThreadUtils.checkInterrupted(e);
+ client.logError("Watcher exception", e);
+ }
+ };
+ }
+ if (watchRunnable != null) {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CURATOR-727: Allow watches to be executed asynchronously [curator]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #514:
URL: https://github.com/apache/curator/pull/514#discussion_r1924124768
##########
curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java:
##########
@@ -445,6 +447,18 @@ public void close() {
unhandledErrorListeners.clear();
connectionStateManager.close();
client.close();
+
+ if (asyncWatchService != null) {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CURATOR-727: Allow watches to be executed asynchronously [curator]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #514:
URL: https://github.com/apache/curator/pull/514#issuecomment-2613761917
cc @eolivelli @kezhuw @Randgalt @cammckenzie
Not quite sure if such a change would break sequential assumption.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org