Extract ThrottledTaskRunner #93436

DaveCTurner · 2023-02-02T07:48:00Z

Generalizes PrioritizedThrottledTaskRunner slightly:

The throttling behaviour is also useful for tasks which do not complete synchronously. The new ThrottledTaskRunner passes a Releasable to each task, which until released will prevent spawning further tasks.
The only part that needs the tasks to be Comparable<> is the queue. Letting the caller specify the queue means that we can also use the throttling without the prioritisation.

Generalizes `PrioritizedThrottledTaskRunner` slightly: - The throttling behaviour is also useful for tasks which do not complete synchronously. The new `ThrottledTaskRunner` passes a `Releasable` to each task, which until released will prevent spawning further tasks. - The only part that needs the tasks to be `Comparable<>` is the queue. Letting the caller specify the queue means that we can also use the throttling without the prioritisation.

elasticsearchmachine · 2023-02-02T07:48:25Z

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

DaveCTurner · 2023-02-02T07:49:16Z

I particularly would like to use this generalisation in #92373, although I think we will find other uses too.

pxsalehi

Thanks David. Added some questions. Aside from using theActionListener<Releasable> there doesn't seem to be a lot of changes in terms of concurrency to the task runner itself? Is that correct?

pxsalehi · 2023-02-02T09:30:07Z

.../test/java/org/elasticsearch/common/util/concurrent/PrioritizedThrottledTaskRunnerTests.java

-    private static void awaitBarrier(CyclicBarrier barrier) {
-        try {
-            barrier.await(10, TimeUnit.SECONDS);
-        } catch (Exception e) {
-            throw new AssertionError("unexpected", e);
-        }
-    }


Aside from getting rid of this, is there any other advantage in introducing TestBarrier?

No, it's just about those pointless checked exceptions.

(I can do this separately too, there's lots of other spots where this is useful)

TBH, I find the class unnecessary. A utility function which takes a timeout parameter will do as well. But I'm fine with leaving it, if you'd like.

Ok, I'll do this in a separate PR

If you find it useful, it's perfectly fine here. I was just trying to understand why we need it at all. 😄

pxsalehi · 2023-02-02T09:31:30Z

...src/test/java/org/elasticsearch/common/util/concurrent/AbstractThrottledTaskRunnerTests.java

+    public void setUp() throws Exception {
+        super.setUp();
+        maxThreads = between(1, 10);
+        executor = EsExecutors.newScaling("test", 1, maxThreads, 0, TimeUnit.MILLISECONDS, false, threadFactory, threadContext);


Does this also need to change like #93446?

pxsalehi · 2023-02-02T09:33:48Z

server/src/main/java/org/elasticsearch/common/util/concurrent/ThrottledTaskRunner.java

+
+import java.util.concurrent.Executor;
+
+public class ThrottledTaskRunner extends AbstractThrottledTaskRunner<ActionListener<Releasable>> {


Is this used anywhere? If not, could we introduce it, when it is needed?

I will be using it in #92373 which is already quite a big change so I'd like to have prerequisites like this reviewed separately. It also came up in a discussion about search execution so I wanted to have the code somewhere visible. I'm happy to delay merging it until #92373 is closer to ready tho.

pxsalehi · 2023-02-06T10:02:06Z

Does this also need to change like #93446?

@DaveCTurner There were two EsExecutors.newScaling executors here in the test which do not seem to have the fix that you had in #93446. Is it not needed here to prevent flaky tests?

The same fix as in elastic#93446, which was missed in elastic#93436

DaveCTurner · 2023-02-06T10:25:29Z

Ah sorry you're right, I forgot about this new test suite. I opened #93505.

The same fix as in #93446, which was missed in #93436

DaveCTurner added :Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. >refactoring v8.7.0 labels Feb 2, 2023

DaveCTurner requested a review from pxsalehi February 2, 2023 07:48

elasticsearchmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Feb 2, 2023

pxsalehi reviewed Feb 2, 2023

View reviewed changes

pxsalehi self-requested a review February 2, 2023 10:19

pxsalehi approved these changes Feb 2, 2023

View reviewed changes

DaveCTurner added 2 commits February 6, 2023 08:15

Merge branch 'main' into 2023-02-02-ThrottledTaskRunner

403b3c3

CyclicBarrierUtils

0aa0ea0

DaveCTurner merged commit e837ff7 into elastic:main Feb 6, 2023

DaveCTurner deleted the 2023-02-02-ThrottledTaskRunner branch February 6, 2023 09:53

DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Feb 6, 2023

AbstractThrottledTaskRunnerTests needs all-core threads

8aa1b9d

The same fix as in elastic#93446, which was missed in elastic#93436

DaveCTurner mentioned this pull request Feb 6, 2023

AbstractThrottledTaskRunnerTests needs all-core threads #93505

Merged

elasticsearchmachine pushed a commit that referenced this pull request Feb 6, 2023

AbstractThrottledTaskRunnerTests needs all-core threads (#93505)

b5387e8

The same fix as in #93446, which was missed in #93436

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract ThrottledTaskRunner #93436

Extract ThrottledTaskRunner #93436

DaveCTurner commented Feb 2, 2023

elasticsearchmachine commented Feb 2, 2023

DaveCTurner commented Feb 2, 2023

pxsalehi left a comment

pxsalehi Feb 2, 2023

DaveCTurner Feb 2, 2023

DaveCTurner Feb 2, 2023

pxsalehi Feb 2, 2023

DaveCTurner Feb 2, 2023

pxsalehi Feb 2, 2023

pxsalehi Feb 2, 2023

DaveCTurner Feb 2, 2023

pxsalehi Feb 2, 2023

DaveCTurner Feb 2, 2023

pxsalehi commented Feb 6, 2023 •

edited

Loading

DaveCTurner commented Feb 6, 2023


		import java.util.concurrent.Executor;

		public class ThrottledTaskRunner extends AbstractThrottledTaskRunner<ActionListener<Releasable>> {

Extract ThrottledTaskRunner #93436

Extract ThrottledTaskRunner #93436

Conversation

DaveCTurner commented Feb 2, 2023

elasticsearchmachine commented Feb 2, 2023

DaveCTurner commented Feb 2, 2023

pxsalehi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pxsalehi commented Feb 6, 2023 • edited Loading

DaveCTurner commented Feb 6, 2023

pxsalehi commented Feb 6, 2023 •

edited

Loading