Refactor MoveDecision method names for clarity #136139
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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.canRemainYes()) { | ||
| moveDecision = balancer.decideRebalance(index, shard, moveDecision.getCanRemainDecision()); | ||
| } | ||
| } | ||
|
|
@@ -819,7 +819,7 @@ public boolean moveShards() { | |
| shardRouting, | ||
| bestNonPreferredShardMovementsTracker::shardIsBetterThanCurrent | ||
| ); | ||
| 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.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.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#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.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.cannotRemainAndCannotMove()) { | ||
| final boolean shardsOnReplacedNode = allocation.metadata().nodeShutdowns().contains(shardRouting.currentNodeId(), REPLACE); | ||
| if (shardsOnReplacedNode) { | ||
| return decideMove(sorter, shardRouting, sourceNode, canRemain, this::decideCanForceAllocateForVacate); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,12 +29,12 @@ public class MoveDecisionTests extends ESTestCase { | |
|
|
||
| public void testCachedDecisions() { | ||
| // cached stay decision | ||
| 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.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.createMoveDecisionWithRemainYesDecision(Decision.YES); | ||
| assertTrue(stay.canRemainYes()); | ||
| assertFalse(stay.cannotRemainAndCanMove()); | ||
| assertTrue(stay.isDecisionTaken()); | ||
| assertNull(stay.getNodeDecisions()); | ||
| assertEquals(AllocationDecision.NO_ATTEMPT, stay.getAllocationDecision()); | ||
|
|
||
| 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.canRemainYes(), readDecision.canRemainYes()); | ||
| assertEquals(moveDecision.getExplanation(), readDecision.getExplanation()); | ||
| assertEquals(moveDecision.cannotRemainAndCanMove(), readDecision.cannotRemainAndCanMove()); | ||
| assertEquals(moveDecision.getNodeDecisions().size(), readDecision.getNodeDecisions().size()); | ||
| assertEquals(moveDecision.getTargetNode(), readDecision.getTargetNode()); | ||
| assertEquals(moveDecision.getAllocationDecision(), readDecision.getAllocationDecision()); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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().canRemainYes() == false | ||
| : "shard [" + pair + "] can remain on node [" + nodeId + "], but that node is shutting down"; | ||
| return pair.v2().getMoveDecision().canRemainYes() == false; | ||
| }) | ||
| // These shards will move as soon as possible | ||
| .filter(pair -> pair.v2().getMoveDecision().getAllocationDecision().equals(AllocationDecision.YES) == false) | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I think such a static method name usually skip the type name, e.g.:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to
createRemainYesDecision.