From 611e33735b282198ffaa2950ac3fc84a34dcc2f1 Mon Sep 17 00:00:00 2001 From: Dianna Hohensee Date: Thu, 30 Oct 2025 12:00:49 -0700 Subject: [PATCH 1/2] Refactor some allocation explain / move decision code --- .../routing/allocation/MoveDecision.java | 17 +++++++++++++---- .../allocator/BalancedShardsAllocator.java | 18 +++++++++--------- .../TransportGetShutdownStatusAction.java | 4 ++-- 3 files changed, 24 insertions(+), 15 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/MoveDecision.java b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/MoveDecision.java index 384b08561d8b0..8c0c39d53a490 100644 --- a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/MoveDecision.java +++ b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/MoveDecision.java @@ -153,7 +153,7 @@ public boolean isDecisionTaken() { */ public boolean cannotRemainAndCanMove() { checkDecisionState(); - return canRemain() == false && canMoveDecision == AllocationDecision.YES; + return cannotRemain() && canMoveDecision == AllocationDecision.YES; } /** @@ -163,7 +163,7 @@ public boolean cannotRemainAndCanMove() { */ public boolean cannotRemainAndCannotMove() { checkDecisionState(); - return canRemain() == false && canMoveDecision != AllocationDecision.YES; + return cannotRemain() && canMoveDecision != AllocationDecision.YES; } /** @@ -175,6 +175,15 @@ public boolean canRemain() { return canRemainDecision.type() == Type.YES; } + /** + * Returns {@code true} if the shard cannot remain on its current node, returns {@code false} if the shard can remain. + * If {@link #isDecisionTaken()} returns {@code false}, then invoking this method will throw an {@code IllegalStateException}. + */ + public boolean cannotRemain() { + checkDecisionState(); + return canRemainDecision.type() != Type.YES; + } + /** * Returns the decision for the shard being allowed to remain on its current node. If {@link #isDecisionTaken()} * returns {@code false}, then invoking this method will throw an {@code IllegalStateException}. @@ -257,7 +266,7 @@ public String getExplanation() { }; } else { // it was a decision to force move the shard - assert canRemain() == false; + assert cannotRemain(); return switch (canMoveDecision) { case YES -> Explanations.Move.YES; case THROTTLED -> Explanations.Move.THROTTLED; @@ -280,7 +289,7 @@ public Iterator toXContentChunked(ToXContent.Params params builder.endObject(); } builder.field("can_remain_on_current_node", canRemain() ? "yes" : "no"); - if (canRemain() == false && canRemainDecision.getDecisions().isEmpty() == false) { + if (cannotRemain() && canRemainDecision.getDecisions().isEmpty() == false) { builder.startArray("can_remain_decisions"); canRemainDecision.toXContent(builder, params); builder.endArray(); diff --git a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java index 865acc26dfe2d..9f4a85d5ee838 100644 --- a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java +++ b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java @@ -259,7 +259,7 @@ public ShardAllocationDecision explainShardAllocation(final ShardRouting shard, } else { moveDecision = balancer.decideMove(index, shard); if (moveDecision.isDecisionTaken() && moveDecision.canRemain()) { - moveDecision = balancer.decideRebalance(index, shard, moveDecision.getCanRemainDecision()); + moveDecision = balancer.explainRebalanceDecision(index, shard, moveDecision.getCanRemainDecision()); } } return new ShardAllocationDecision(allocateUnassignedDecision, moveDecision); @@ -465,7 +465,7 @@ private boolean balance() { * optimally balanced cluster. This method is invoked from the cluster allocation * explain API only. */ - private MoveDecision decideRebalance(final ProjectIndex index, final ShardRouting shard, Decision canRemain) { + private MoveDecision explainRebalanceDecision(final ProjectIndex index, final ShardRouting shard, Decision canRemain) { final NodeSorter sorter = nodeSorters.sorterForShard(shard); index.assertMatch(shard); if (shard.started() == false) { @@ -832,7 +832,7 @@ public boolean moveShards() { } shardMoved = true; } - } else if (moveDecision.isDecisionTaken() && moveDecision.canRemain() == false) { + } else if (moveDecision.isDecisionTaken() && moveDecision.cannotRemain()) { logger.trace("[{}][{}] can't move", shardRouting.index(), shardRouting.id()); } } @@ -925,13 +925,13 @@ private MoveDecision decideMove(ProjectIndex index, ShardRouting shardRouting, P final ModelNode sourceNode = nodes.get(shardRouting.currentNodeId()); assert sourceNode != null && sourceNode.containsShard(index, shardRouting); RoutingNode routingNode = sourceNode.getRoutingNode(); - Decision canRemain = allocation.deciders().canRemain(shardRouting, routingNode, allocation); - if (canRemain.type() != Decision.Type.NO && canRemain.type() != Decision.Type.NOT_PREFERRED) { - return MoveDecision.createRemainYesDecision(canRemain); + Decision canRemainDecision = allocation.deciders().canRemain(shardRouting, routingNode, allocation); + if (canRemainDecision.type() != Decision.Type.NO && canRemainDecision.type() != Decision.Type.NOT_PREFERRED) { + return MoveDecision.createRemainYesDecision(canRemainDecision); } // Check predicate to decide whether to assess movement options - if (canRemain.type() == Type.NOT_PREFERRED && nonPreferredPredicate.test(shardRouting) == false) { + if (canRemainDecision.type() == Type.NOT_PREFERRED && nonPreferredPredicate.test(shardRouting) == false) { return MoveDecision.NOT_TAKEN; } @@ -942,11 +942,11 @@ private MoveDecision decideMove(ProjectIndex index, ShardRouting shardRouting, P * This is not guaranteed to be balanced after this operation we still try best effort to * allocate on the minimal eligible node. */ - final MoveDecision moveDecision = decideMove(sorter, shardRouting, sourceNode, canRemain, this::decideCanAllocate); + final MoveDecision moveDecision = decideMove(sorter, shardRouting, sourceNode, canRemainDecision, this::decideCanAllocate); if (moveDecision.cannotRemainAndCannotMove()) { final boolean shardsOnReplacedNode = allocation.metadata().nodeShutdowns().contains(shardRouting.currentNodeId(), REPLACE); if (shardsOnReplacedNode) { - return decideMove(sorter, shardRouting, sourceNode, canRemain, this::decideCanForceAllocateForVacate); + return decideMove(sorter, shardRouting, sourceNode, canRemainDecision, this::decideCanForceAllocateForVacate); } } return moveDecision; diff --git a/x-pack/plugin/shutdown/src/main/java/org/elasticsearch/xpack/shutdown/TransportGetShutdownStatusAction.java b/x-pack/plugin/shutdown/src/main/java/org/elasticsearch/xpack/shutdown/TransportGetShutdownStatusAction.java index b95847bb3cf02..d052f0f1e5842 100644 --- a/x-pack/plugin/shutdown/src/main/java/org/elasticsearch/xpack/shutdown/TransportGetShutdownStatusAction.java +++ b/x-pack/plugin/shutdown/src/main/java/org/elasticsearch/xpack/shutdown/TransportGetShutdownStatusAction.java @@ -285,9 +285,9 @@ static ShutdownShardMigrationStatus shardMigrationStatus( .map(shardRouting -> new Tuple<>(shardRouting, allocationService.explainShardAllocation(shardRouting, allocation))) // Given that we're checking the status of a node that's shutting down, no shards should be allowed to remain .filter(pair -> { - assert pair.v2().getMoveDecision().canRemain() == false + assert pair.v2().getMoveDecision().cannotRemain() : "shard [" + pair + "] can remain on node [" + nodeId + "], but that node is shutting down"; - return pair.v2().getMoveDecision().canRemain() == false; + return pair.v2().getMoveDecision().cannotRemain(); }) // These shards will move as soon as possible .filter(pair -> pair.v2().getMoveDecision().getAllocationDecision().equals(AllocationDecision.YES) == false) From f6938fa558c5da8c51dbd6974e639587b85d13ea Mon Sep 17 00:00:00 2001 From: Dianna Hohensee Date: Fri, 7 Nov 2025 12:38:09 -0800 Subject: [PATCH 2/2] call canRemain from cannotRemain --- .../elasticsearch/cluster/routing/allocation/MoveDecision.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/MoveDecision.java b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/MoveDecision.java index 8c0c39d53a490..af00d13814155 100644 --- a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/MoveDecision.java +++ b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/MoveDecision.java @@ -180,8 +180,7 @@ public boolean canRemain() { * If {@link #isDecisionTaken()} returns {@code false}, then invoking this method will throw an {@code IllegalStateException}. */ public boolean cannotRemain() { - checkDecisionState(); - return canRemainDecision.type() != Type.YES; + return canRemain() == false; } /**