Refactor MoveDecision method names for clarity (#136139)

These method names reflect the purpose for which they were used in the
past, rather than what they mean. Given how much the code has changed
since class/methods were created, and the different ways in which they
are now used, the names have become confusing. This commit changes some
names to better reflect meaning.

Author: DiannaHohensee

server/src/internalClusterTest/java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainIT.java

@@ -539,7 +539,7 @@ public void testAllocationFilteringPreventsShardMove() throws Exception {
         assertEquals(AllocationDecision.NO, moveDecision.getAllocationDecision());
         assertEquals(Explanations.Move.NO, moveDecision.getExplanation());
         assertFalse(moveDecision.canRemain());
-        assertFalse(moveDecision.forceMove());
+        assertFalse(moveDecision.cannotRemainAndCanMove());
         assertFalse(moveDecision.canRebalanceCluster());
         assertNull(moveDecision.getClusterRebalanceDecision());
         assertNull(moveDecision.getTargetNode());
@@ -652,7 +652,7 @@ public void testRebalancingNotAllowed() throws Exception {
         assertEquals(AllocationDecision.NO, moveDecision.getAllocationDecision());
         assertEquals(Explanations.Rebalance.CANNOT_REBALANCE_CAN_ALLOCATE, moveDecision.getExplanation());
         assertTrue(moveDecision.canRemain());
-        assertFalse(moveDecision.forceMove());
+        assertFalse(moveDecision.cannotRemainAndCanMove());
         assertFalse(moveDecision.canRebalanceCluster());
         assertNotNull(moveDecision.getCanRemainDecision());
         assertNull(moveDecision.getTargetNode());
@@ -758,7 +758,7 @@ public void testWorseBalance() throws Exception {
         assertEquals(AllocationDecision.NO, moveDecision.getAllocationDecision());
         assertEquals(Explanations.Rebalance.ALREADY_BALANCED, moveDecision.getExplanation());
         assertTrue(moveDecision.canRemain());
-        assertFalse(moveDecision.forceMove());
+        assertFalse(moveDecision.cannotRemainAndCanMove());
         assertTrue(moveDecision.canRebalanceCluster());
         assertNotNull(moveDecision.getCanRemainDecision());
         assertNull(moveDecision.getTargetNode());
@@ -856,7 +856,7 @@ public void testBetterBalanceButCannotAllocate() throws Exception {
         assertEquals(AllocationDecision.NO, moveDecision.getAllocationDecision());
         assertEquals(Explanations.Rebalance.ALREADY_BALANCED, moveDecision.getExplanation());
         assertTrue(moveDecision.canRemain());
-        assertFalse(moveDecision.forceMove());
+        assertFalse(moveDecision.cannotRemainAndCanMove());
         assertTrue(moveDecision.canRebalanceCluster());
         assertNotNull(moveDecision.getCanRemainDecision());
         assertNull(moveDecision.getTargetNode());
@@ -964,7 +964,7 @@ public void testAssignedReplicaOnSpecificNode() throws Exception {
         assertEquals(AllocationDecision.NO, moveDecision.getAllocationDecision());
         assertEquals(Explanations.Rebalance.CANNOT_REBALANCE_CANNOT_ALLOCATE, moveDecision.getExplanation());
         assertTrue(moveDecision.canRemain());
-        assertFalse(moveDecision.forceMove());
+        assertFalse(moveDecision.cannotRemainAndCanMove());
         assertFalse(moveDecision.canRebalanceCluster());
         assertNotNull(moveDecision.getCanRemainDecision());
         assertNull(moveDecision.getTargetNode());

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

@@ -92,11 +92,12 @@ 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 createRemainYesDecision(Decision canRemainDecision) {
+        assert canRemainDecision.type() != Type.NO;
+        assert canRemainDecision.type() != Type.NOT_PREFERRED;
         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,11 +151,21 @@ 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 canRemain() == 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;
+    }
+
     /**
      * 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}.
@@ -287,7 +298,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;

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

@@ -821,7 +821,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);
@@ -845,7 +845,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;
@@ -898,8 +898,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.
          *
@@ -926,7 +927,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.createRemainYesDecision(canRemain);
             }
 
             // Check predicate to decide whether to assess movement options
@@ -942,7 +943,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);

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.createRemainYesDecision(Decision.YES);
+        MoveDecision stay2 = MoveDecision.createRemainYesDecision(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.createRemainYesDecision(new Decision.Single(Type.YES, null, null, (Object[]) null));
+        stay2 = MoveDecision.createRemainYesDecision(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);
+        MoveDecision stay = MoveDecision.createRemainYesDecision(Decision.YES);
         assertTrue(stay.canRemain());
-        assertFalse(stay.forceMove());
+        assertFalse(stay.cannotRemainAndCanMove());
         assertTrue(stay.isDecisionTaken());
         assertNull(stay.getNodeDecisions());
         assertEquals(AllocationDecision.NO_ATTEMPT, stay.getAllocationDecision());
 
-        stay = MoveDecision.remain(Decision.YES);
+        stay = MoveDecision.createRemainYesDecision(Decision.YES);
         assertTrue(stay.canRemain());
-        assertFalse(stay.forceMove());
+        assertFalse(stay.cannotRemainAndCanMove());
         assertTrue(stay.isDecisionTaken());
         assertNull(stay.getNodeDecisions());
         assertEquals(AllocationDecision.NO_ATTEMPT, stay.getAllocationDecision());
@@ -116,7 +116,7 @@ public void testSerialization() throws IOException {
         MoveDecision readDecision = new MoveDecision(output.bytes().streamInput());
         assertEquals(moveDecision.canRemain(), readDecision.canRemain());
         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());