Refactor MoveDecision method names for clarity

DiannaHohensee · DiannaHohensee · commit bbd6002227e5 · 2025-10-07T17:33:17.000-04:00
diff --git a/server/src/internalClusterTest/java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainIT.java b/server/src/internalClusterTest/java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainIT.java
@@ -538,8 +538,8 @@ public void testAllocationFilteringPreventsShardMove() throws Exception {
         assertTrue(moveDecision.isDecisionTaken());
         assertEquals(AllocationDecision.NO, moveDecision.getAllocationDecision());
         assertEquals(Explanations.Move.NO, moveDecision.getExplanation());
-        assertFalse(moveDecision.canRemain());
-        assertFalse(moveDecision.forceMove());
+        assertFalse(moveDecision.canRemainYes());
+        assertFalse(moveDecision.cannotRemainAndCanMove());
         assertFalse(moveDecision.canRebalanceCluster());
         assertNull(moveDecision.getClusterRebalanceDecision());
         assertNull(moveDecision.getTargetNode());
@@ -651,8 +651,8 @@ public void testRebalancingNotAllowed() throws Exception {
         assertTrue(moveDecision.isDecisionTaken());
         assertEquals(AllocationDecision.NO, moveDecision.getAllocationDecision());
         assertEquals(Explanations.Rebalance.CANNOT_REBALANCE_CAN_ALLOCATE, moveDecision.getExplanation());
-        assertTrue(moveDecision.canRemain());
-        assertFalse(moveDecision.forceMove());
+        assertTrue(moveDecision.canRemainYes());
+        assertFalse(moveDecision.cannotRemainAndCanMove());
         assertFalse(moveDecision.canRebalanceCluster());
         assertNotNull(moveDecision.getCanRemainDecision());
         assertNull(moveDecision.getTargetNode());
@@ -757,8 +757,8 @@ public void testWorseBalance() throws Exception {
         assertTrue(moveDecision.isDecisionTaken());
         assertEquals(AllocationDecision.NO, moveDecision.getAllocationDecision());
         assertEquals(Explanations.Rebalance.ALREADY_BALANCED, moveDecision.getExplanation());
-        assertTrue(moveDecision.canRemain());
-        assertFalse(moveDecision.forceMove());
+        assertTrue(moveDecision.canRemainYes());
+        assertFalse(moveDecision.cannotRemainAndCanMove());
         assertTrue(moveDecision.canRebalanceCluster());
         assertNotNull(moveDecision.getCanRemainDecision());
         assertNull(moveDecision.getTargetNode());
@@ -855,8 +855,8 @@ public void testBetterBalanceButCannotAllocate() throws Exception {
         assertTrue(moveDecision.isDecisionTaken());
         assertEquals(AllocationDecision.NO, moveDecision.getAllocationDecision());
         assertEquals(Explanations.Rebalance.ALREADY_BALANCED, moveDecision.getExplanation());
-        assertTrue(moveDecision.canRemain());
-        assertFalse(moveDecision.forceMove());
+        assertTrue(moveDecision.canRemainYes());
+        assertFalse(moveDecision.cannotRemainAndCanMove());
         assertTrue(moveDecision.canRebalanceCluster());
         assertNotNull(moveDecision.getCanRemainDecision());
         assertNull(moveDecision.getTargetNode());
@@ -963,8 +963,8 @@ public void testAssignedReplicaOnSpecificNode() throws Exception {
         assertTrue(moveDecision.isDecisionTaken());
         assertEquals(AllocationDecision.NO, moveDecision.getAllocationDecision());
         assertEquals(Explanations.Rebalance.CANNOT_REBALANCE_CANNOT_ALLOCATE, moveDecision.getExplanation());
-        assertTrue(moveDecision.canRemain());
-        assertFalse(moveDecision.forceMove());
+        assertTrue(moveDecision.canRemainYes());
+        assertFalse(moveDecision.cannotRemainAndCanMove());
         assertFalse(moveDecision.canRebalanceCluster());
         assertNotNull(moveDecision.getCanRemainDecision());
         assertNull(moveDecision.getTargetNode());
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
@@ -92,11 +92,11 @@ public void writeTo(StreamOutput out) throws IOException {
      * Creates a move decision for the shard being able to remain on its current node, so the shard won't
      * be forced to move to another node.
      */
-    public static MoveDecision remain(Decision canRemainDecision) {
+    public static MoveDecision createMoveDecisionWithRemainYesDecision(Decision canRemainDecision) {
+        assert canRemainDecision.type() != Type.NO;
         if (canRemainDecision == Decision.YES) {
             return CACHED_STAY_DECISION;
         }
-        assert canRemainDecision.type() != Type.NO;
         return new MoveDecision(null, null, AllocationDecision.NO_ATTEMPT, canRemainDecision, null, 0);
     }
 
@@ -150,16 +150,26 @@ public boolean isDecisionTaken() {
      * returns {@code false} otherwise.  If {@link #isDecisionTaken()} returns {@code false},
      * then invoking this method will throw an {@code IllegalStateException}.
      */
-    public boolean forceMove() {
+    public boolean cannotRemainAndCanMove() {
+        checkDecisionState();
+        return canRemainYes() == false && canMoveDecision == AllocationDecision.YES;
+    }
+
+    /**
+     * Returns {@code true} if the shard cannot remain on its current node and _cannot_ be moved.
+     * returns {@code false} otherwise.  If {@link #isDecisionTaken()} returns {@code false},
+     * then invoking this method will throw an {@code IllegalStateException}.
+     */
+    public boolean cannotRemainAndCannotMove() {
         checkDecisionState();
-        return canRemain() == false && canMoveDecision == AllocationDecision.YES;
+        return canRemainDecision.type() != Type.YES && canMoveDecision != AllocationDecision.YES;
     }
 
     /**
      * Returns {@code true} if the shard can remain on its current node, returns {@code false} otherwise.
      * If {@link #isDecisionTaken()} returns {@code false}, then invoking this method will throw an {@code IllegalStateException}.
      */
-    public boolean canRemain() {
+    public boolean canRemainYes() {
         checkDecisionState();
         return canRemainDecision.type() == Type.YES;
     }
@@ -187,7 +197,7 @@ public boolean canRebalanceCluster() {
 
     /**
      * Returns the decision for being allowed to rebalance the shard.  Invoking this method will return
-     * {@code null} if {@link #canRemain()} ()} returns {@code false}, which means the node is not allowed to
+     * {@code null} if {@link #canRemainYes()} ()} returns {@code false}, which means the node is not allowed to
      * remain on its current node, so the cluster is forced to attempt to move the shard to a different node,
      * as opposed to attempting to rebalance the shard if a better cluster balance is possible by moving it.
      * If {@link #isDecisionTaken()} returns {@code false}, then invoking this method will throw an
@@ -246,7 +256,7 @@ public String getExplanation() {
             };
         } else {
             // it was a decision to force move the shard
-            assert canRemain() == false;
+            assert canRemainYes() == false;
             return switch (canMoveDecision) {
                 case YES -> Explanations.Move.YES;
                 case THROTTLED -> Explanations.Move.THROTTLED;
@@ -268,8 +278,8 @@ public Iterator<? extends ToXContent> toXContentChunked(ToXContent.Params params
                 discoveryNodeToXContent(targetNode, true, builder);
                 builder.endObject();
             }
-            builder.field("can_remain_on_current_node", canRemain() ? "yes" : "no");
-            if (canRemain() == false && canRemainDecision.getDecisions().isEmpty() == false) {
+            builder.field("can_remain_on_current_node", canRemainYes() ? "yes" : "no");
+            if (canRemainYes() == false && canRemainDecision.getDecisions().isEmpty() == false) {
                 builder.startArray("can_remain_decisions");
                 canRemainDecision.toXContent(builder, params);
                 builder.endArray();
@@ -287,7 +297,7 @@ public Iterator<? extends ToXContent> toXContentChunked(ToXContent.Params params
                 builder.field("can_rebalance_to_other_node", canMoveDecision);
                 builder.field("rebalance_explanation", getExplanation());
             } else {
-                builder.field("can_move_to_other_node", forceMove() ? "yes" : "no");
+                builder.field("can_move_to_other_node", cannotRemainAndCanMove() ? "yes" : "no");
                 builder.field("move_explanation", getExplanation());
             }
             return builder;
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
@@ -258,7 +258,7 @@ public ShardAllocationDecision decideShardAllocation(final ShardRouting shard, f
             allocateUnassignedDecision = balancer.decideAllocateUnassigned(index, shard);
         } else {
             moveDecision = balancer.decideMove(index, shard);
-            if (moveDecision.isDecisionTaken() && moveDecision.canRemain()) {
+            if (moveDecision.isDecisionTaken() && moveDecision.canRemainYes()) {
                 moveDecision = balancer.decideRebalance(index, shard, moveDecision.getCanRemainDecision());
             }
         }
@@ -819,7 +819,7 @@ public boolean moveShards() {
                     shardRouting,
                     bestNonPreferredShardMovementsTracker::shardIsBetterThanCurrent
                 );
-                if (moveDecision.isDecisionTaken() && moveDecision.forceMove()) {
+                if (moveDecision.isDecisionTaken() && moveDecision.cannotRemainAndCanMove()) {
                     // Defer moving of not-preferred until we've moved the NOs
                     if (moveDecision.getCanRemainDecision().type() == Type.NOT_PREFERRED) {
                         bestNonPreferredShardMovementsTracker.putBestMoveDecision(shardRouting, moveDecision);
@@ -830,7 +830,7 @@ public boolean moveShards() {
                         }
                         shardMoved = true;
                     }
-                } else if (moveDecision.isDecisionTaken() && moveDecision.canRemain() == false) {
+                } else if (moveDecision.isDecisionTaken() && moveDecision.canRemainYes() == false) {
                     logger.trace("[{}][{}] can't move", shardRouting.index(), shardRouting.id());
                 }
             }
@@ -843,7 +843,7 @@ public boolean moveShards() {
                 // invalid, so we must call `decideMove` again. If not, we know we haven't made any moves, and we
                 // can use the cached decision.
                 final var moveDecision = shardMoved ? decideMove(index, shardRouting) : storedShardMovement.moveDecision();
-                if (moveDecision.isDecisionTaken() && moveDecision.forceMove()) {
+                if (moveDecision.isDecisionTaken() && moveDecision.cannotRemainAndCanMove()) {
                     executeMove(shardRouting, index, moveDecision, "move-non-preferred");
                     // Return after a single move so that the change can be simulated before further moves are made.
                     return true;
@@ -896,8 +896,9 @@ public MoveDecision decideMove(final ProjectIndex index, final ShardRouting shar
          *   2. If the shard is allowed to remain on its current node, no attempt will be made to move the shard and
          *      {@link MoveDecision#getCanRemainDecision} will have a decision type of YES. All other fields in the object will be null.
          *   3. If the shard is not allowed to remain on its current node, then {@link MoveDecision#getAllocationDecision()} will be
-         *      populated with the decision of moving to another node. If {@link MoveDecision#forceMove()} returns {@code true}, then
-         *      {@link MoveDecision#getTargetNode} will return a non-null value, otherwise the assignedNodeId will be null.
+         *      populated with the decision of moving to another node. If {@link MoveDecision#cannotRemainAndCanMove()} returns
+         *      {@code true}, then {@link MoveDecision#getTargetNode} will return a non-null value, otherwise the assignedNodeId will be
+         *      null.
          *   4. If the method is invoked in explain mode (e.g. from the cluster allocation explain APIs), then
          *      {@link MoveDecision#getNodeDecisions} will have a non-null value.
          *
@@ -924,7 +925,7 @@ private MoveDecision decideMove(ProjectIndex index, ShardRouting shardRouting, P
             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.remain(canRemain);
+                return MoveDecision.createMoveDecisionWithRemainYesDecision(canRemain);
             }
 
             // Check predicate to decide whether to assess movement options
@@ -940,7 +941,7 @@ private MoveDecision decideMove(ProjectIndex index, ShardRouting shardRouting, P
              * allocate on the minimal eligible node.
              */
             final MoveDecision moveDecision = decideMove(sorter, shardRouting, sourceNode, canRemain, this::decideCanAllocate);
-            if (moveDecision.canRemain() == false && moveDecision.forceMove() == false) {
+            if (moveDecision.cannotRemainAndCannotMove()) {
                 final boolean shardsOnReplacedNode = allocation.metadata().nodeShutdowns().contains(shardRouting.currentNodeId(), REPLACE);
                 if (shardsOnReplacedNode) {
                     return decideMove(sorter, shardRouting, sourceNode, canRemain, this::decideCanForceAllocateForVacate);
diff --git a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/MoveDecisionTests.java b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/MoveDecisionTests.java
@@ -29,12 +29,12 @@ public class MoveDecisionTests extends ESTestCase {
 
     public void testCachedDecisions() {
         // cached stay decision
-        MoveDecision stay1 = MoveDecision.remain(Decision.YES);
-        MoveDecision stay2 = MoveDecision.remain(Decision.YES);
+        MoveDecision stay1 = MoveDecision.createMoveDecisionWithRemainYesDecision(Decision.YES);
+        MoveDecision stay2 = MoveDecision.createMoveDecisionWithRemainYesDecision(Decision.YES);
         assertSame(stay1, stay2); // not in explain mode, so should use cached decision
 
-        stay1 = MoveDecision.remain(new Decision.Single(Type.YES, null, null, (Object[]) null));
-        stay2 = MoveDecision.remain(new Decision.Single(Type.YES, null, null, (Object[]) null));
+        stay1 = MoveDecision.createMoveDecisionWithRemainYesDecision(new Decision.Single(Type.YES, null, null, (Object[]) null));
+        stay2 = MoveDecision.createMoveDecisionWithRemainYesDecision(new Decision.Single(Type.YES, null, null, (Object[]) null));
         assertNotSame(stay1, stay2);
 
         // cached cannot move decision
@@ -57,16 +57,16 @@ public void testCachedDecisions() {
     }
 
     public void testStayDecision() {
-        MoveDecision stay = MoveDecision.remain(Decision.YES);
-        assertTrue(stay.canRemain());
-        assertFalse(stay.forceMove());
+        MoveDecision stay = MoveDecision.createMoveDecisionWithRemainYesDecision(Decision.YES);
+        assertTrue(stay.canRemainYes());
+        assertFalse(stay.cannotRemainAndCanMove());
         assertTrue(stay.isDecisionTaken());
         assertNull(stay.getNodeDecisions());
         assertEquals(AllocationDecision.NO_ATTEMPT, stay.getAllocationDecision());
 
-        stay = MoveDecision.remain(Decision.YES);
-        assertTrue(stay.canRemain());
-        assertFalse(stay.forceMove());
+        stay = MoveDecision.createMoveDecisionWithRemainYesDecision(Decision.YES);
+        assertTrue(stay.canRemainYes());
+        assertFalse(stay.cannotRemainAndCanMove());
         assertTrue(stay.isDecisionTaken());
         assertNull(stay.getNodeDecisions());
         assertEquals(AllocationDecision.NO_ATTEMPT, stay.getAllocationDecision());
@@ -114,9 +114,9 @@ public void testSerialization() throws IOException {
         BytesStreamOutput output = new BytesStreamOutput();
         moveDecision.writeTo(output);
         MoveDecision readDecision = new MoveDecision(output.bytes().streamInput());
-        assertEquals(moveDecision.canRemain(), readDecision.canRemain());
+        assertEquals(moveDecision.canRemainYes(), readDecision.canRemainYes());
         assertEquals(moveDecision.getExplanation(), readDecision.getExplanation());
-        assertEquals(moveDecision.forceMove(), readDecision.forceMove());
+        assertEquals(moveDecision.cannotRemainAndCanMove(), readDecision.cannotRemainAndCanMove());
         assertEquals(moveDecision.getNodeDecisions().size(), readDecision.getNodeDecisions().size());
         assertEquals(moveDecision.getTargetNode(), readDecision.getTargetNode());
         assertEquals(moveDecision.getAllocationDecision(), readDecision.getAllocationDecision());
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
@@ -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().canRemainYes() == false
                     : "shard [" + pair + "] can remain on node [" + nodeId + "], but that node is shutting down";
-                return pair.v2().getMoveDecision().canRemain() == false;
+                return pair.v2().getMoveDecision().canRemainYes() == false;
             })
             // These shards will move as soon as possible
             .filter(pair -> pair.v2().getMoveDecision().getAllocationDecision().equals(AllocationDecision.YES) == false)
