Add an option to return early from an allocate call by ywangd · Pull Request #134786 · elastic/elasticsearch

ywangd · 2025-09-16T09:04:37Z

Instead of running simulation all the way to balance or no possible movement, this PR adds an option to finish early based on the number of relocating shards. Note this early return mechanism is enabled only when desired balance is in use, i.e. it affects simulation only.

Resolves: ES-12862

Instead of running simulation all the way to balance or no possible movement, this PR adds an option to finish early based on the number of relocating shards. Resolves: ES-12862

ywangd · 2025-09-16T09:05:32Z

Note to reviewers: This is a draft PR to ensure that I am on the right track and align with other relevant changes. Your feedback is appreciated! 🙏

DiannaHohensee

Left some feedback/questions.

I'm primarily concerned with the ability to configure the number of relocations before returning. The purpose of this change is to avoid ever encountering THROTTLE answers that can negatively interact with NOT_PREFERRED, resulting in allocation of a shard to NOT_PREFERRED because all the other nodes returned THROTTLE (not the decision we want to make). If the number of relocations is configurable, then we can run into throttling after 2 relocations from the ThrottlingAllocationDecider and/or ConcurrentRebalanceAllocationDecider -- maybe I'm missing other THROTTLE cases.

Additionally, maybe we can sprinkle some assertions around the balancer code that we never encounter a THROTTLE answer? It would be bad if we did 😬

...ain/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java

DiannaHohensee · 2025-09-16T21:28:35Z

...ain/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java

+                            if (allocation.hasTargetRelocatingShards()) {
+                                // The shard may have been relocated on the ModelNode, but not on the cluster (RoutingNodes) if
+                                // it is throttled. The `hasTargetRelocatingShards` check does not account for this case.
+                                // This might be a non-issue since our intention is to have no Throttle with early returns.


The supposed contract of tryRelocateShard, according to the method comment, is that true means the move was executed "on the cluster" 🤔 Pre-existing outdated comment, seems like.

How about we update this code

elasticsearch/server/src/main/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java

Lines 1152 to 1155 in 7d6e1e4

canAllocateOrRebalance == Type.YES

/* only allocate on the cluster if we are not throttled */

? routingNodes.relocateShard(shard, minNode.getNodeId(), shardSize, "rebalance", allocation.changes()).v1()

: shard.relocate(minNode.getNodeId(), shardSize)

to assert a non-throttling answer and always do the routingNodes relocate call? With your complete code changes for this task, I would never expect the balancer to run into THROTTLE again -- we're trying to avoid THROTTLE situations, which can lead to overriding not-preferred answers.

Yeah added such an assertion in the caller, see dbd0fed

...ain/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java

server/src/main/java/org/elasticsearch/cluster/routing/allocation/RoutingAllocation.java

...main/java/org/elasticsearch/cluster/routing/allocation/allocator/DesiredBalanceComputer.java

This reverts commit 4aad2a3.

...ain/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java

ywangd · 2025-09-17T06:17:08Z

The PR is now updated based on the latest discussion, specifically:

Whether to return early depends only on the balancer type and not otherwise configurable
Relevant methods now return boolean to indicate movements/assignements
Added a few more assertions
Fixed tests which depend on the BalancedShardAllocator behaviours.

Your comments are appreciated. Thank you!

…-from-allocate

nicktindall

LGTM

...ain/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java

…-from-allocate

ywangd · 2025-09-18T01:49:14Z

...ain/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java

+        assert allocation.isSimulating() == false || balancerSettings.completeEarlyOnShardAssignmentChange()
+            : "inconsistent states: isSimulating ["
+                + allocation.isSimulating()
+                + "] vs completeEarlyOnShardAssignmentChange ["
+                + balancerSettings.completeEarlyOnShardAssignmentChange()
+                + "]";


Based on the production code flows, the BalancedShardsAllocator should only see simulating RoutingAllocation when DesiredBalanceAllocator is in use. Otherwise it should always see a non-simulating RoutingAllocation. Therefore, we could potentially just use allocation.isSimulating() and avoid the need for balancerSettings.completeEarlyOnShardAssignmentChange(). But I decided to keep it for now because:

They are conceptually separate things

Instead of ensuring them being identical at all times, we only really need to ensure simulating RoutingAllocation can only be handled by a balancer with completeEarlyOnShardAssignmentChange() == true, i.e. what the assertion checks.

It's easy to remove completeEarlyOnShardAssignmentChange in future when we are sure there is no any exception (production or test). For now, it feels safer with it.

elasticsearchmachine · 2025-09-18T01:49:48Z

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

ywangd · 2025-09-18T01:50:24Z

@nicktindall I added a test in bd00382. It would be great if you could give the PR another look. Thanks a lot!

nicktindall

Test LGTM also!

henningandersen

LGTM.

…134786)" This reverts commit 43741c2.

#135476) This reverts commit 43741c2.

…#134786)" (elastic#135476) This reverts commit 31f1810.

This PR reapply #134786 with a bug fix which previously caused many test failures. Resolves: #135406 Resolves: #135407 Resolves: #135150 Resolves: #135151 Resolves: #135194 Resolves: #135248 Resolves: #135249 Resolves: #135408 Resolves: #135473 Resolves: #135474 The following is the original commit message Instead of running simulation all the way to balance or no possible movement, this PR adds an option to finish early based on the number of relocating shards. Note this early return mechanism is enabled only when desired balance is in use, i.e. it affects simulation only. Resolves: ES-12862

Add an option to return early from an allocate call

4aad2a3

Instead of running simulation all the way to balance or no possible movement, this PR adds an option to finish early based on the number of relocating shards. Resolves: ES-12862

ywangd requested review from DiannaHohensee, mhl-b and nicktindall September 16, 2025 09:04

ywangd added >non-issue :Distributed/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) v9.2.0 labels Sep 16, 2025

ywangd mentioned this pull request Sep 16, 2025

Implement move non preferred phase in allocator #134429

Closed

DiannaHohensee reviewed Sep 16, 2025

View reviewed changes

Revert "Add an option to return early from an allocate call"

4716915

This reverts commit 4aad2a3.

nicktindall reviewed Sep 16, 2025

View reviewed changes

...ain/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java Outdated Show resolved Hide resolved

ywangd added 3 commits September 17, 2025 12:33

A different take

dbd0fed

Merge branch 'main' into ES-12862-return-early-from-allocate

567f667

fix tests

044ebd7

ywangd requested review from DiannaHohensee and nicktindall September 17, 2025 06:17

ywangd added 2 commits September 17, 2025 16:50

suppress wider

3afceb2

Merge remote-tracking branch 'origin/main' into ES-12862-return-early…

6238ce7

…-from-allocate

nicktindall approved these changes Sep 17, 2025

View reviewed changes

...ain/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java Outdated Show resolved Hide resolved

ywangd added 4 commits September 17, 2025 18:13

move declaration

52f96d3

Merge remote-tracking branch 'origin/main' into ES-12862-return-early…

ea0fb5d

…-from-allocate

Merge remote-tracking branch 'origin/main' into ES-12862-return-early…

8976ac5

…-from-allocate

Add test

bd00382

ywangd commented Sep 18, 2025

View reviewed changes

ywangd marked this pull request as ready for review September 18, 2025 01:49

elasticsearchmachine added the Team:Distributed Coordination (obsolete) Meta label for Distributed Coordination team. Obsolete. Please do not use. label Sep 18, 2025

ywangd requested a review from nicktindall September 18, 2025 01:50

ywangd requested a review from henningandersen September 18, 2025 01:54

nicktindall approved these changes Sep 18, 2025

View reviewed changes

henningandersen approved these changes Sep 18, 2025

View reviewed changes

Merge branch 'main' into ES-12862-return-early-from-allocate

091b90b

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

ywangd added 2 commits September 19, 2025 10:36

Soften assertion

80300b1

add JIRA issue

4977339

elasticsearchmachine merged commit 43741c2 into elastic:main Sep 19, 2025
34 checks passed

ywangd deleted the ES-12862-return-early-from-allocate branch September 19, 2025 02:13

lkts mentioned this pull request Sep 22, 2025

[CI] RollingUpgradeSearchableSnapshotIndexCompatibilityIT testMountSearchableSnapshot {p0=[9.2.0, 9.2.0, 9.2.0]} failing #135151

Closed

joshua-adams-1 mentioned this pull request Sep 23, 2025

[CI] DesiredBalanceComputerTests testDesiredBalanceShouldConvergeInABigCluster failing #135123

Closed

nicktindall mentioned this pull request Sep 24, 2025

allocation: create separate limit for frozen tier concurrent rebalance #135243

Merged

masseyke mentioned this pull request Sep 25, 2025

[CI] DataStreamsUpgradeIT testDataStreamValidationDoesNotBreakUpgrade failing #135406

Closed

DiannaHohensee added a commit to DiannaHohensee/elasticsearch that referenced this pull request Sep 25, 2025

Revert "Add an option to return early from an allocate call (elastic#…

d0a8d71

…134786)" This reverts commit 43741c2.

elasticsearchmachine pushed a commit that referenced this pull request Sep 25, 2025

Revert "Add an option to return early from an allocate call (#134786)" (

31f1810

#135476) This reverts commit 43741c2.

ywangd added a commit to ywangd/elasticsearch that referenced this pull request Sep 29, 2025

Reapply "Add an option to return early from an allocate call (elastic…

1ba1a5c

…#134786)" (elastic#135476) This reverts commit 31f1810.

ywangd mentioned this pull request Sep 29, 2025

Reapply "Add an option to return early from an allocate call" #135589

Merged

nicktindall mentioned this pull request Nov 25, 2025

Add unthrottled path for replicas in ThrottlingAllocationDecider #138545

Merged

	canAllocateOrRebalance == Type.YES
	/* only allocate on the cluster if we are not throttled */
	? routingNodes.relocateShard(shard, minNode.getNodeId(), shardSize, "rebalance", allocation.changes()).v1()
	: shard.relocate(minNode.getNodeId(), shardSize)

Conversation

ywangd commented Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ywangd commented Sep 16, 2025

Uh oh!

DiannaHohensee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DiannaHohensee Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

ywangd Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ywangd commented Sep 17, 2025

Uh oh!

nicktindall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ywangd Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Sep 18, 2025

Uh oh!

ywangd commented Sep 18, 2025

Uh oh!

nicktindall left a comment

Choose a reason for hiding this comment

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ywangd commented Sep 16, 2025 •

edited

Loading