React more prompty to task cancellation while waiting for the cluster to unblock

[nielsbauman]

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

[elasticsearchmachine]

Hi @nielsbauman, I've created a changelog YAML for you.

[elasticsearchmachine]

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

[DaveCTurner]

Looks good, just a naming/comment nit.

[DaveCTurner]

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.

[nielsbauman]

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.

[DaveCTurner]

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.

[nielsbauman]

Alright, renamed to waitComplete in 3c6aafa.

[DaveCTurner]

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();
+    }
 }

[nielsbauman]

Great suggestion, thanks! Applied in 0925808.

[DaveCTurner]

LGTM still