Create an end-to-end IT test for WriteLoadConstraintDecider#canAllocate #133500

DiannaHohensee · 2025-08-25T21:59:05Z

Closes ES-12620

…t assert non-immutable

elasticsearchmachine · 2025-08-25T22:02:31Z

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

DiannaHohensee · 2025-08-25T22:02:55Z

server/src/main/java/org/elasticsearch/cluster/NodeUsageStatsForThreadPools.java


    public NodeUsageStatsForThreadPools(StreamInput in) throws IOException {
-        this(in.readString(), in.readMap(ThreadPoolUsageStats::new));
+        this(in.readString(), in.readImmutableMap(ThreadPoolUsageStats::new));


I had to change the NodeUsageStatsForThreadPool constructor to read into an immutable map because the ShardMovements*Simulator expects an immutable map.

nicktindall

LGTM, just some nits

nicktindall · 2025-08-26T23:03:59Z

.../java/org/elasticsearch/cluster/routing/allocation/decider/WriteLoadConstraintDeciderIT.java

+    protected Collection<Class<? extends Plugin>> getMockPlugins() {
+        final Collection<Class<? extends Plugin>> plugins = new ArrayList<>(super.getMockPlugins());
+        plugins.add(MockTransportService.TestPlugin.class);
+        return plugins;


Nit: maybe

return CollectionUtils.appendToCopy(super.nodePlugins(), MockTransportService.TestPlugin.class);

Sure, done 👍 bb30938

nicktindall · 2025-08-26T23:11:46Z

.../java/org/elasticsearch/cluster/routing/allocation/decider/WriteLoadConstraintDeciderIT.java

+            indexName,
+            Settings.builder().put(SETTING_NUMBER_OF_SHARDS, randomNumberOfShards).put(SETTING_NUMBER_OF_REPLICAS, 0).build()
+        );
+        index(indexName, Integer.toString(randomInt(10)), Collections.singletonMap("foo", "bar"));


Nit: maybe

indexRandom(false, indexName, 1);

to make it clear the ID and doc is not important?

In fact is this even necessary? or does createIndex create all the shards?

Shouldn't be necessary, good point. Removed and it works fine 👍

nicktindall · 2025-08-26T23:18:14Z

.../java/org/elasticsearch/cluster/routing/allocation/decider/WriteLoadConstraintDeciderIT.java

+        MockTransportService.getInstance(secondDataNodeName)
+            .addRequestHandlingBehavior(IndicesStatsAction.NAME + "[n]", (handler, request, channel, task) -> {
+                // Return no stats for the index because none are assigned to this node.
+                TransportIndicesStatsAction instance = internalCluster().getInstance(TransportIndicesStatsAction.class, firstDataNodeName);


Should this be

internalCluster().getInstance(TransportIndicesStatsAction.class, secondDataNodeName);

Also below on the third one (maybe copy-paste error)? It probably doesn't matter because it doesn't look like NodeResponse references anything from the enclosing class when used this way, but it's a bit confusing.

Indeed it should be! Thanks!

nicktindall · 2025-08-26T23:24:57Z

.../java/org/elasticsearch/cluster/routing/allocation/decider/WriteLoadConstraintDeciderIT.java

+         */
+
+        logger.info("---> Refreshing the cluster info to pull in the dummy thread pool stats with a hot-spotting node");
+        final InternalClusterInfoService clusterInfoService = (InternalClusterInfoService) internalCluster().getInstance(


Nit: maybe

final InternalClusterInfoService clusterInfoService = asInstanceOf(InternalClusterService.class, internalCluster.getInstance(...

Just to make it fail with an assertion error if it's not what we expect

Nice, done 👍

nicktindall · 2025-08-26T23:31:02Z

.../java/org/elasticsearch/cluster/routing/allocation/decider/WriteLoadConstraintDeciderIT.java

+        DiscoveryNode discoveryNode,
+        int totalWriteThreadPoolThreads,
+        float averageWriteThreadPoolUtilization,
+        long averageWriteThreadPoolQueueLatencyMillis


Isn't it max write thread pool queue latency?

Copy-paste takes another victim. Fixed. Thanks!

nicktindall · 2025-08-26T23:33:32Z

.../java/org/elasticsearch/cluster/routing/allocation/decider/WriteLoadConstraintDeciderIT.java

+            averageWriteThreadPoolQueueLatencyMillis
+        );
+        var threadPoolUsageMap = new HashMap<String, NodeUsageStatsForThreadPools.ThreadPoolUsageStats>();
+        threadPoolUsageMap.put(ThreadPool.Names.WRITE, writeThreadPoolUsageStats);


Nit: maybe this is more succinct (and also results in an immutable singleton map)?

var threadPoolUsageMap = Map.of(ThreadPool.Names.WRITE, new NodeUsageStatsForThreadPools.ThreadPoolUsageStats( totalWriteThreadPoolThreads, averageWriteThreadPoolUtilization, averageWriteThreadPoolQueueLatencyMillis );

Sure, immutable sounds like a win too 👍

nicktindall · 2025-08-27T00:25:55Z

.../java/org/elasticsearch/cluster/routing/allocation/decider/WriteLoadConstraintDeciderIT.java

+
+    /**
+     * Helper to create a dummy {@link ShardStats} for the given index shard with the supplied {@code peekWriteLoad} value.
+     */


Nit: peak rather than peek?

nicktindall · 2025-08-27T00:35:44Z

.../java/org/elasticsearch/cluster/routing/allocation/decider/WriteLoadConstraintDeciderIT.java

+            .put(
+                WriteLoadConstraintSettings.WRITE_LOAD_DECIDER_ENABLED_SETTING.getKey(),
+                WriteLoadConstraintSettings.WriteLoadDeciderStatus.ENABLED
+            )


Can we set the write load threshold explicitly (and randomly) and calculate the above/below thresholds based on that below? We're relying on defaults as it stands and they might change.

Sure. IIUC, you're also proposing randomizing the node's reported thread pool utilization to somewhere above the random threshold? Updated as such. 5d3dd54

DiannaHohensee

Fixed up 👍 Thanks for the review Nick.

DiannaHohensee · 2025-08-28T21:59:28Z

.../java/org/elasticsearch/cluster/routing/allocation/decider/WriteLoadConstraintDeciderIT.java

+
+    /**
+     * Helper to create a dummy {@link ShardStats} for the given index shard with the supplied {@code peekWriteLoad} value.
+     */


DiannaHohensee · 2025-08-28T21:59:32Z

.../java/org/elasticsearch/cluster/routing/allocation/decider/WriteLoadConstraintDeciderIT.java

+            averageWriteThreadPoolQueueLatencyMillis
+        );
+        var threadPoolUsageMap = new HashMap<String, NodeUsageStatsForThreadPools.ThreadPoolUsageStats>();
+        threadPoolUsageMap.put(ThreadPool.Names.WRITE, writeThreadPoolUsageStats);


Sure, immutable sounds like a win too 👍

DiannaHohensee · 2025-08-28T21:59:36Z

.../java/org/elasticsearch/cluster/routing/allocation/decider/WriteLoadConstraintDeciderIT.java

+        DiscoveryNode discoveryNode,
+        int totalWriteThreadPoolThreads,
+        float averageWriteThreadPoolUtilization,
+        long averageWriteThreadPoolQueueLatencyMillis


Copy-paste takes another victim. Fixed. Thanks!

DiannaHohensee · 2025-08-28T21:59:44Z

.../java/org/elasticsearch/cluster/routing/allocation/decider/WriteLoadConstraintDeciderIT.java

+         */
+
+        logger.info("---> Refreshing the cluster info to pull in the dummy thread pool stats with a hot-spotting node");
+        final InternalClusterInfoService clusterInfoService = (InternalClusterInfoService) internalCluster().getInstance(


Nice, done 👍

DiannaHohensee · 2025-08-28T21:59:49Z

.../java/org/elasticsearch/cluster/routing/allocation/decider/WriteLoadConstraintDeciderIT.java

+        MockTransportService.getInstance(secondDataNodeName)
+            .addRequestHandlingBehavior(IndicesStatsAction.NAME + "[n]", (handler, request, channel, task) -> {
+                // Return no stats for the index because none are assigned to this node.
+                TransportIndicesStatsAction instance = internalCluster().getInstance(TransportIndicesStatsAction.class, firstDataNodeName);


Indeed it should be! Thanks!

DiannaHohensee · 2025-08-28T21:59:59Z

.../java/org/elasticsearch/cluster/routing/allocation/decider/WriteLoadConstraintDeciderIT.java

+            indexName,
+            Settings.builder().put(SETTING_NUMBER_OF_SHARDS, randomNumberOfShards).put(SETTING_NUMBER_OF_REPLICAS, 0).build()
+        );
+        index(indexName, Integer.toString(randomInt(10)), Collections.singletonMap("foo", "bar"));


Shouldn't be necessary, good point. Removed and it works fine 👍

DiannaHohensee · 2025-08-28T22:00:05Z

.../java/org/elasticsearch/cluster/routing/allocation/decider/WriteLoadConstraintDeciderIT.java

+    protected Collection<Class<? extends Plugin>> getMockPlugins() {
+        final Collection<Class<? extends Plugin>> plugins = new ArrayList<>(super.getMockPlugins());
+        plugins.add(MockTransportService.TestPlugin.class);
+        return plugins;


Sure, done 👍 bb30938

DiannaHohensee · 2025-08-28T22:35:38Z

.../java/org/elasticsearch/cluster/routing/allocation/decider/WriteLoadConstraintDeciderIT.java

+            .put(
+                WriteLoadConstraintSettings.WRITE_LOAD_DECIDER_ENABLED_SETTING.getKey(),
+                WriteLoadConstraintSettings.WriteLoadDeciderStatus.ENABLED
+            )


Sure. IIUC, you're also proposing randomizing the node's reported thread pool utilization to somewhere above the random threshold? Updated as such. 5d3dd54

DiannaHohensee added 8 commits August 25, 2025 14:48

Testing still needs to set up shard write load dummy values

d4d5a85

finished test, not run yet

d165802

debugging test

a02290f

test runs successfully, needs lots of cleanup

699d6e2

test fixed up

e5b4842

make map immutable so that the simulator copy-and-replace call doesn'…

5dfefcc

…t assert non-immutable

improve comment

a4c5d47

undo public constructor

abcc58e

DiannaHohensee self-assigned this Aug 25, 2025

elasticsearchmachine added needs:triage Requires assignment of a team area label v9.2.0 labels Aug 25, 2025

DiannaHohensee commented Aug 25, 2025

View reviewed changes

remove empty line

aa30fdb

DiannaHohensee added the >test Issues or PRs that are addressing/adding tests label Aug 25, 2025

DiannaHohensee requested review from DaveCTurner and nicktindall August 25, 2025 23:05

nicktindall approved these changes Aug 27, 2025

View reviewed changes

DiannaHohensee added 5 commits August 28, 2025 14:37

Merge branch 'main' into 2025/08/21/ES-12620-IT-Test

57312b2

use helper for plugin addition

bb30938

most of the review changes

274a164

randomize utilization threshold setting

5d3dd54

Merge branch 'main' into 2025/08/21/ES-12620-IT-Test

352ddda

DiannaHohensee commented Aug 28, 2025

View reviewed changes

Merge branch 'main' into 2025/08/21/ES-12620-IT-Test

02d1de1

DiannaHohensee added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Aug 29, 2025

elasticsearchmachine merged commit ae3134d into elastic:main Aug 29, 2025
33 checks passed

DiannaHohensee deleted the 2025/08/21/ES-12620-IT-Test branch August 29, 2025 19:40

Create an end-to-end IT test for WriteLoadConstraintDecider#canAllocate #133500

Create an end-to-end IT test for WriteLoadConstraintDecider#canAllocate #133500

Uh oh!

Conversation

DiannaHohensee commented Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Aug 25, 2025

Uh oh!

DiannaHohensee Aug 25, 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!

nicktindall 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!

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!

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!

DiannaHohensee 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DiannaHohensee commented Aug 25, 2025 •

edited

Loading

DiannaHohensee Aug 25, 2025 •

edited

Loading