Start indexing throttling only after disk IO unthrottling does not keep up with the merge load #125654

albertzaharovits · 2025-03-26T11:12:53Z

Fixes an issue where indexing throttling kicks in while disk IO is throttling.
Instead disk IO should first unthrottle, and only then, if we still can't keep up with the merging load, start throttling indexing.

Fixes https://github.com/elastic/elasticsearch-benchmarks/issues/2437
Relates #120869

elasticsearchmachine · 2025-03-26T11:16:19Z

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

server/src/main/java/org/elasticsearch/index/engine/ThreadPoolMergeScheduler.java

albertzaharovits · 2025-03-27T13:49:23Z

server/src/main/java/org/elasticsearch/index/engine/ThreadPoolMergeScheduler.java

@@ -50,7 +50,7 @@ public class ThreadPoolMergeScheduler extends MergeScheduler implements Elastics
    );
    private final ShardId shardId;
    private final MergeSchedulerConfig config;
-    private final Logger logger;
+    protected final Logger logger;


Unrelated change, in order to let these log messages go to the ThreadPoolMergeScheduler's logger instead of the InternalEngine's.

henningandersen

LGTM, though I think we can maybe simplify the test?

henningandersen · 2025-03-27T16:04:20Z

server/src/test/java/org/elasticsearch/index/engine/ThreadPoolMergeSchedulerTests.java

+            BiConsumer<Integer, Integer> enableIndexingThrottlingHook,
+            BiConsumer<Integer, Integer> disableIndexingThrottlingHook


Looks like these can be just Runnable which would remove a bit of code elsewhere too.

I've removed these, the TestThreadPoolMergeScheduler directly exposes "if index throttling" as a boolean.

henningandersen · 2025-03-27T16:10:48Z

server/src/test/java/org/elasticsearch/index/engine/ThreadPoolMergeSchedulerTests.java

+        int excessMerges = randomIntBetween(1, 10);
+        int mergesToSubmit = maxMergeCount + mergesToRun + excessMerges;
+        int mergesOutstanding = 0;
+        boolean expectIndexThrottling = false;


It is not clear to me that every test run invokes index throttling. I wonder if we are overcomplicating the test a little here and whether we should simply make it have X merges submitted and then verify that index-throttling kicks in (twice, one with the max-io-rate-simulation set to false first, then one where it is set to true)?

I agree, it's complicated and it not always tests that index throttling kicks in when IO throttling is at max level.
But, I still think the test is valuable because it constantly asserts the indexing throttling state while merges are submitted, scheduler, and running at the same time (and it's deterministic).

I've added another test, that asserts indexing throttling kicks in when more than max_merge_count merges are submitted (some are scheduled, but none is run). It's simpler, it always asserts that index throttling is toggled, but it's covering less.

Let me know if this now looks OK with you.

fressi-elastic

Despite I spotted some details that look "concerns" to me, I would prefer other person with more time in the project would review this code again, before blocking the merge process.
Thank you for this quick patch.

fressi-elastic · 2025-03-28T11:17:45Z

server/src/test/java/org/elasticsearch/index/engine/ThreadPoolMergeSchedulerTests.java

@@ -173,6 +173,165 @@ public void testSimpleMergeTaskReEnqueueingBySize() {
        }
    }

+    public void testIndexingThrottlingWhenSubmittingMerges() {
+        final int maxThreadCount = randomIntBetween(1, 5);


Instead of testing with random values, I would recommend running a parametrized test with different parameters set so that we can have reproducible results.

This is not so much about parameterized testing as it is about just not hard-coding a specific value. It is quite common to do this in our code-base, so I'd prefer to keep this as is.

fressi-elastic · 2025-03-28T11:24:10Z

server/src/test/java/org/elasticsearch/index/engine/ThreadPoolMergeSchedulerTests.java

+        int submittedMerges = 0;
+        // merges are submitted, while some are also scheduled (but none is run)
+        while (submittedMerges < mergesToSubmit - 1) {
+            isUsingMaxTargetIORate.set(randomBoolean());


The same as above. I think having success with random number in place could give a false sense of healthy state, that could later end up in failure. I would instead suggest having a parametrized test case with all the combination of parameters sets considered to be interesting for testing. I would suggest some reading regarding table driven unit tests [1]

[1] https://semaphore.io/blog/table-driven-unit-tests-go

I think it is ok here, since the test ends up verifying that throttling does kick in when true and the loop here is expected to run a few times in most cases.

fressi-elastic · 2025-03-28T11:27:52Z

server/src/test/java/org/elasticsearch/index/engine/ThreadPoolMergeSchedulerTests.java

@@ -493,4 +652,49 @@ private static MergeInfo getNewMergeInfo(long estimatedMergeBytes) {
    private static MergeInfo getNewMergeInfo(long estimatedMergeBytes, int maxNumSegments) {
        return new MergeInfo(randomNonNegativeInt(), estimatedMergeBytes, randomBoolean(), maxNumSegments);
    }
+
+    static class TestThreadPoolMergeScheduler extends ThreadPoolMergeScheduler {


If you have to override a class to test it (or for it), in this case, probably what you need is a field you can change in the original class itself to trigger a different behaviour. The reason is a refactor in the code could give unexpected side effects not easy to spot by not testing agains the actual implementation.

We typically try to avoid such artificial test pieces in production code. I think such an override as here is quite common so do not mind the current form. Feel free to maybe open a draft PR after this is merged to illustrate your point more (I am not sure I fully understood it).

henningandersen

LGTM.

…ep up with the merge load (elastic#125654) Fixes an issue where indexing throttling kicks in while disk IO is throttling. Instead disk IO should first unthrottle, and only then, if we still can't keep up with the merging load, start throttling indexing. Fixes elastic/elasticsearch-benchmarks#2437 Relates elastic#120869

Only start indexing throttling when disk throttling is lowest

7e7e624

albertzaharovits self-assigned this Mar 26, 2025

elasticsearchmachine added v9.1.0 needs:triage Requires assignment of a team area label labels Mar 26, 2025

albertzaharovits added :Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. >non-issue and removed needs:triage Requires assignment of a team area label labels Mar 26, 2025

elasticsearchmachine added the Team:Distributed Indexing Meta label for Distributed Indexing team label Mar 26, 2025

elasticsearchmachine and others added 2 commits March 26, 2025 11:21

[CI] Auto commit changes from spotless

5c3ef42

Ooops

54cc5ab

henningandersen reviewed Mar 26, 2025

View reviewed changes

server/src/main/java/org/elasticsearch/index/engine/ThreadPoolMergeScheduler.java Outdated Show resolved Hide resolved

testIndexingThrottling

d1b8193

albertzaharovits requested a review from henningandersen March 27, 2025 13:45

albertzaharovits commented Mar 27, 2025

View reviewed changes

elasticsearchmachine and others added 2 commits March 27, 2025 13:51

[CI] Auto commit changes from spotless

67fe301

Checkstyle

774389d

albertzaharovits added :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. and removed :Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. labels Mar 27, 2025

Merge branch 'main' into throttle-only-when-IO-rate-is-high

0c8c149

henningandersen reviewed Mar 27, 2025

View reviewed changes

albertzaharovits and others added 7 commits March 28, 2025 08:11

TestThreadPoolMergeScheduler

7864c46

mockThreadPoolMergeExecutorService

948da81

testIndexingThrottlingWhileMergesAreRunning

e381634

isUsingMaxTargetIORate

f41cb27

Merge branch 'main' into throttle-only-when-IO-rate-is-high

0a91ab6

[CI] Auto commit changes from spotless

5472e6a

testIndexingThrottlingWhenSubmittingMerges

a419917

albertzaharovits requested a review from henningandersen March 28, 2025 08:40

fressi-elastic reviewed Mar 28, 2025

View reviewed changes

henningandersen approved these changes Mar 28, 2025

View reviewed changes

albertzaharovits added 2 commits March 30, 2025 22:39

Merge branch 'main' into throttle-only-when-IO-rate-is-high

ea84548

Fix IndexStatsIT.testThrottleStats

c4cf8b0

albertzaharovits merged commit 84e1372 into elastic:main Mar 30, 2025
17 checks passed

albertzaharovits deleted the throttle-only-when-IO-rate-is-high branch March 30, 2025 21:26

This was referenced Jun 9, 2025

[9.0] New threadpool-based merge scheduler which is disk space aware #129134

Merged

[8.19] New threadpool-based merge scheduler which is disk space aware #129152

Merged

		BiConsumer<Integer, Integer> enableIndexingThrottlingHook,
		BiConsumer<Integer, Integer> disableIndexingThrottlingHook

Start indexing throttling only after disk IO unthrottling does not keep up with the merge load #125654

Start indexing throttling only after disk IO unthrottling does not keep up with the merge load #125654

Uh oh!

Conversation

albertzaharovits commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Mar 26, 2025

Uh oh!

Uh oh!

albertzaharovits Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fressi-elastic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

albertzaharovits commented Mar 26, 2025 •

edited

Loading

albertzaharovits Mar 27, 2025 •

edited

Loading