React more prompty to task cancellation while waiting for the cluster to unblock #128737
Conversation
… to unblock Instead of waiting for the next run of the `ClusterStateObserver` (which might be arbitrarily far in the future, but bound by the timeout if one is set), we notify the listener immediately that the task has been cancelled. While doing so, we ensure we invoke the listener only once. Fixes elastic#117971
nielsbauman
added
>enhancement
:Distributed Coordination/Task Management
|
Hi @nielsbauman, I've created a changelog YAML for you. |
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
| @@ -104,20 +106,40 @@ private void waitForClusterUnblock( | |||
| logger, | |||
| clusterService.threadPool().getThreadContext() | |||
| ); | |||
| // We track whether we already notified the listener of cancellation, to avoid invoking the listener twice. | |||
| final var notifiedCancellation = new AtomicBoolean(false); | |||
There was a problem hiding this comment.
Naming is a bit odd, this gets set to true even if we haven't notified the listener of cancellation. Maybe waitComplete?
I was going to suggest using org.elasticsearch.action.ActionListener#notifyOnce but it's more subtle than that: we want to suppress the cancellation listener before calling innerDoExecute. I think that deserves a comment.
There was a problem hiding this comment.
I renamed the variable to notifiedListener. Even though we don't notify the listener directly in onNewClusterState, we do in innerDoExecute. I also updated the comment. Let me know what you think.
There was a problem hiding this comment.
Yeah I think that's still going to be confusing to some future reader. The flag doesn't indicate we've completed the listener, it indicates that the wait for an appropriate cluster state in TransportLocalClusterStateAction is over and we've started to execute the action, so I think waitComplete would be a better name.
There was a problem hiding this comment.
Alright, renamed to waitComplete in 3c6aafa.
There was a problem hiding this comment.
LGTM, but also consider extracting a utility for this pattern to simplify the compareAndSet things for future readers. ... (false, true) == false is pretty hard to parse. Something like this perhaps?
diff --git a/libs/core/src/main/java/org/elasticsearch/core/Predicates.java b/libs/core/src/main/java/org/elasticsearch/core/Predicates.java
index bd8c1517323..88c4f138967 100644
--- a/libs/core/src/main/java/org/elasticsearch/core/Predicates.java
+++ b/libs/core/src/main/java/org/elasticsearch/core/Predicates.java
@@ -9,6 +9,8 @@
package org.elasticsearch.core;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.function.BooleanSupplier;
import java.util.function.Predicate;
/**
@@ -90,4 +92,22 @@ public enum Predicates {
public static <T> Predicate<T> never() {
return (Predicate<T>) NEVER;
}
+
+ private static class OnceTrue extends AtomicBoolean implements BooleanSupplier {
+ OnceTrue() {
+ super(true);
+ }
+
+ @Override
+ public boolean getAsBoolean() {
+ return getAndSet(false);
+ }
+ }
+
+ /**
+ * @return a {@link BooleanSupplier} which supplies {@code true} the first time it is called, and {@code false} subsequently.
+ */
+ public static BooleanSupplier once() {
+ return new OnceTrue();
+ }
}
|
Great suggestion, thanks! Applied in 0925808. |
… to unblock (elastic#128737) Instead of waiting for the next run of the `ClusterStateObserver` (which might be arbitrarily far in the future, but bound by the timeout if one is set), we notify the listener immediately that the task has been cancelled. While doing so, we ensure we invoke the listener only once. Fixes elastic#117971
… to unblock (elastic#128737) Instead of waiting for the next run of the `ClusterStateObserver` (which might be arbitrarily far in the future, but bound by the timeout if one is set), we notify the listener immediately that the task has been cancelled. While doing so, we ensure we invoke the listener only once. Fixes elastic#117971
Instead of waiting for the next run of the
ClusterStateObserver(which might be arbitrarily far in the future, but bound by the timeout if one is set), we notify the listener immediately that the task has been cancelled. While doing so, we ensure we invoke the listener only once.Fixes #117971