Add WriteLoadDecider canRemain logic #135326

DiannaHohensee · 2025-09-23T22:39:50Z

Closes ES-12633

elasticsearchmachine · 2025-09-23T22:46:24Z

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

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

nicktindall

LGTM with some minor cosmetic nits

Making utility methods for the repetitive set-up will keep the body of the test method focused on the behaviour being tested.

nicktindall · 2025-09-30T07:20:25Z

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

+                (handler, request, channel, task) -> channel.sendResponse(
+                    new NodeUsageStatsForThreadPoolsAction.NodeResponse(harness.thirdDiscoveryNode, thirdNodeUtilHotSpottingNodeStats)
+                )
+            );


Nit: this could be DRY'd, e.g.

mockNodeUsageStats(harness.thirdDiscoveryNode, thirdNodeUtilHotSpottingNodeStats)

Fair point, done! 93a133a

nicktindall · 2025-09-30T07:29:25Z

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

+                    harness.thirdDataNodeName
+                );
+                channel.sendResponse(instance.new NodeResponse(harness.thirdDataNodeId, 0, List.of(), List.of()));
+            });


Also this ^^^

Done 👍 4978f5b & 3e5604e

nicktindall · 2025-09-30T07:31:19Z

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

+                Level.DEBUG,
+                "Desired balance computation for * is completed, scheduling reconciliation"
+            )
+        );


Also this ? ^^^

Sure, done 👍 9a5fe8d

Actually, perhaps you meant the whole MockLog.awaitLogger statement? It looks more concise with MockLog.SeenEventExpectation modularized. I think I prefer to see the actions that the test is taking -- refreshClusterInfo and updateClusterSettings -- so I left that alone.

nicktindall · 2025-09-30T07:33:06Z

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

+                (handler, request, channel, task) -> channel.sendResponse(
+                    new NodeUsageStatsForThreadPoolsAction.NodeResponse(harness.thirdDiscoveryNode, thirdNodeNonHotSpottingNodeStats)
+                )
+            );


nicktindall · 2025-09-30T07:33:57Z

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

+                    harness.thirdDataNodeName
+                );
+                channel.sendResponse(instance.new NodeResponse(harness.thirdDataNodeId, 0, List.of(), List.of()));
+            });


nicktindall · 2025-09-30T07:34:17Z

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

+                Level.DEBUG,
+                "Desired balance updated for *"
+            )
+        );


nicktindall · 2025-09-30T07:38:10Z

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

+                TimeValue.timeValueMillis(queueLatencyThresholdMillis)
+            )
+            // Essentially disable rebalancing so that testing can see Decider change outcomes.
+            .put(BalancedShardsAllocator.THRESHOLD_SETTING.getKey(), 1000f)


Nit: Could you use EnableAllocationDecider#CLUSTER_ROUTING_REBALANCE_ENABLE_SETTING to be more explicit?

Yep, that looks better 👍 592cd08

nicktindall · 2025-09-30T07:45:13Z

...in/java/org/elasticsearch/cluster/routing/allocation/decider/WriteLoadConstraintDecider.java

+
+        if (logger.isTraceEnabled()) {
+            logger.trace(explanation);
+        }


Nit: I don't think we need to wrap this in the conditional, log4j will do that immediately inside logger.trace(...). Explicit checks like this are only useful to avoid expensive pre-calculation or fetching that may be required for the log message. Here we're just passing in a string that already exists (i.e. zero cost).

Good point, copy-paste for the fail 🙂 Fixed 👍 2fe4115

…elper method

…le rebalancing entirely

…g test failures

DiannaHohensee

Updated. Good idea to modularize the transport action overrides, it looks better now.

DiannaHohensee · 2025-10-01T14:33:31Z

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

+                (handler, request, channel, task) -> channel.sendResponse(
+                    new NodeUsageStatsForThreadPoolsAction.NodeResponse(harness.thirdDiscoveryNode, thirdNodeUtilHotSpottingNodeStats)
+                )
+            );


Fair point, done! 93a133a

DiannaHohensee · 2025-10-01T15:00:47Z

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

+                    harness.thirdDataNodeName
+                );
+                channel.sendResponse(instance.new NodeResponse(harness.thirdDataNodeId, 0, List.of(), List.of()));
+            });


Done 👍 4978f5b & 3e5604e

DiannaHohensee · 2025-10-01T15:08:31Z

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

+                Level.DEBUG,
+                "Desired balance computation for * is completed, scheduling reconciliation"
+            )
+        );


Sure, done 👍 9a5fe8d

DiannaHohensee · 2025-10-01T15:11:59Z

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

+                Level.DEBUG,
+                "Desired balance computation for * is completed, scheduling reconciliation"
+            )
+        );


Actually, perhaps you meant the whole MockLog.awaitLogger statement? It looks more concise with MockLog.SeenEventExpectation modularized. I think I prefer to see the actions that the test is taking -- refreshClusterInfo and updateClusterSettings -- so I left that alone.

DiannaHohensee · 2025-10-01T15:19:37Z

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

+                TimeValue.timeValueMillis(queueLatencyThresholdMillis)
+            )
+            // Essentially disable rebalancing so that testing can see Decider change outcomes.
+            .put(BalancedShardsAllocator.THRESHOLD_SETTING.getKey(), 1000f)


Yep, that looks better 👍 592cd08

DiannaHohensee · 2025-10-01T15:21:21Z

...in/java/org/elasticsearch/cluster/routing/allocation/decider/WriteLoadConstraintDecider.java

+
+        if (logger.isTraceEnabled()) {
+            logger.trace(explanation);
+        }


Good point, copy-paste for the fail 🙂 Fixed 👍 2fe4115

DiannaHohensee added 3 commits September 23, 2025 14:19

Add WriteLoadDecider canRemain logic

295a09e

remove throttling work arounds; rearrange some racy asserts

205a1a6

clean up

1b259be

DiannaHohensee self-assigned this Sep 23, 2025

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

fix message string

7ba085a

add unit testing for canRemain

5a35ed4

DiannaHohensee commented Sep 24, 2025

View reviewed changes

.../java/org/elasticsearch/cluster/routing/allocation/decider/WriteLoadConstraintDeciderIT.java Show resolved Hide resolved

DiannaHohensee commented Sep 24, 2025

View reviewed changes

.../java/org/elasticsearch/cluster/routing/allocation/decider/WriteLoadConstraintDeciderIT.java Show resolved Hide resolved

DiannaHohensee added 3 commits September 29, 2025 17:48

Merge branch 'main' into 2025/09/10/ES-12633-canRemain-rebased

382774e

update mockLog usage to helper methods

34fc7e3

spotless

5b4bde3

DiannaHohensee requested a review from nicktindall September 29, 2025 22:34

nicktindall approved these changes Sep 30, 2025

View reviewed changes

DiannaHohensee added 8 commits October 1, 2025 10:35

refactor TransportNodeUsageStatsForThreadPoolsAction overrides into h…

93a133a

…elper method

refactor IndicesStatsAction overrides into helper method

4978f5b

add helper to create list of ShardStats

3e5604e

create a helper method for MockLog.SeenEventExpectation events

9a5fe8d

replace very high rebalancing threshold setting with setting to disab…

592cd08

…le rebalancing entirely

remove isTraceEnabled() around a trace log

2fe4115

Merge branch 'main' into 2025/09/10/ES-12633-canRemain-rebased

b89c4a9

add some expectations around shard count -- though it was not trippin…

f8e16f3

…g test failures

DiannaHohensee commented Oct 1, 2025

View reviewed changes

[CI] Auto commit changes from spotless

8e51ec9

DiannaHohensee added 2 commits October 1, 2025 13:03

remove assert of shard write load estimate collection size

20d85f9

Merge branch 'main' into 2025/09/10/ES-12633-canRemain-rebased

5d891fa

DiannaHohensee merged commit 9fb01e1 into elastic:main Oct 1, 2025
34 checks passed

Add WriteLoadDecider canRemain logic #135326

Add WriteLoadDecider canRemain logic #135326

Conversation

DiannaHohensee commented Sep 23, 2025

Uh oh!

elasticsearchmachine commented Sep 23, 2025

Uh oh!

Uh oh!

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!

DiannaHohensee left a comment • 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!

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 left a comment •

edited

Loading