Skip to content

Commit 0de0dce

Browse files
Refactor some allocation explain / move decision code (#137400)
Add `MoveDecision#cannotRemain()` to replace the numerous `canRemain() == false` checks Rename `Balancer#decideRebalance()` to `Balancer#explainRebalanceDecision()` Rename a `Decision canRemain` variable to `Decision canRemainDecision` so it doesn't sound like a boolean Relates ES-12833
1 parent 3ae5ee1 commit 0de0dce

File tree

3 files changed

+23
-15
lines changed

3 files changed

+23
-15
lines changed

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

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ public boolean isDecisionTaken() {
153153
*/
154154
public boolean cannotRemainAndCanMove() {
155155
checkDecisionState();
156-
return canRemain() == false && canMoveDecision == AllocationDecision.YES;
156+
return cannotRemain() && canMoveDecision == AllocationDecision.YES;
157157
}
158158

159159
/**
@@ -163,7 +163,7 @@ public boolean cannotRemainAndCanMove() {
163163
*/
164164
public boolean cannotRemainAndCannotMove() {
165165
checkDecisionState();
166-
return canRemain() == false && canMoveDecision != AllocationDecision.YES;
166+
return cannotRemain() && canMoveDecision != AllocationDecision.YES;
167167
}
168168

169169
/**
@@ -175,6 +175,14 @@ public boolean canRemain() {
175175
return canRemainDecision.type() == Type.YES;
176176
}
177177

178+
/**
179+
* Returns {@code true} if the shard cannot remain on its current node, returns {@code false} if the shard can remain.
180+
* If {@link #isDecisionTaken()} returns {@code false}, then invoking this method will throw an {@code IllegalStateException}.
181+
*/
182+
public boolean cannotRemain() {
183+
return canRemain() == false;
184+
}
185+
178186
/**
179187
* Returns the decision for the shard being allowed to remain on its current node. If {@link #isDecisionTaken()}
180188
* returns {@code false}, then invoking this method will throw an {@code IllegalStateException}.
@@ -257,7 +265,7 @@ public String getExplanation() {
257265
};
258266
} else {
259267
// it was a decision to force move the shard
260-
assert canRemain() == false;
268+
assert cannotRemain();
261269
return switch (canMoveDecision) {
262270
case YES -> Explanations.Move.YES;
263271
case THROTTLED -> Explanations.Move.THROTTLED;
@@ -280,7 +288,7 @@ public Iterator<? extends ToXContent> toXContentChunked(ToXContent.Params params
280288
builder.endObject();
281289
}
282290
builder.field("can_remain_on_current_node", canRemain() ? "yes" : "no");
283-
if (canRemain() == false && canRemainDecision.getDecisions().isEmpty() == false) {
291+
if (cannotRemain() && canRemainDecision.getDecisions().isEmpty() == false) {
284292
builder.startArray("can_remain_decisions");
285293
canRemainDecision.toXContent(builder, params);
286294
builder.endArray();

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

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -259,7 +259,7 @@ public ShardAllocationDecision explainShardAllocation(final ShardRouting shard,
259259
} else {
260260
moveDecision = balancer.decideMove(index, shard);
261261
if (moveDecision.isDecisionTaken() && moveDecision.canRemain()) {
262-
moveDecision = balancer.decideRebalance(index, shard, moveDecision.getCanRemainDecision());
262+
moveDecision = balancer.explainRebalanceDecision(index, shard, moveDecision.getCanRemainDecision());
263263
}
264264
}
265265
return new ShardAllocationDecision(allocateUnassignedDecision, moveDecision);
@@ -465,7 +465,7 @@ private boolean balance() {
465465
* optimally balanced cluster. This method is invoked from the cluster allocation
466466
* explain API only.
467467
*/
468-
private MoveDecision decideRebalance(final ProjectIndex index, final ShardRouting shard, Decision canRemain) {
468+
private MoveDecision explainRebalanceDecision(final ProjectIndex index, final ShardRouting shard, Decision canRemain) {
469469
final NodeSorter sorter = nodeSorters.sorterForShard(shard);
470470
index.assertMatch(shard);
471471
if (shard.started() == false) {
@@ -852,7 +852,7 @@ public boolean moveShards() {
852852
}
853853
shardMoved = true;
854854
}
855-
} else if (moveDecision.isDecisionTaken() && moveDecision.canRemain() == false) {
855+
} else if (moveDecision.isDecisionTaken() && moveDecision.cannotRemain()) {
856856
logger.trace("[{}][{}] can't move", shardRouting.index(), shardRouting.id());
857857
}
858858
}
@@ -945,13 +945,13 @@ private MoveDecision decideMove(ProjectIndex index, ShardRouting shardRouting, P
945945
final ModelNode sourceNode = nodes.get(shardRouting.currentNodeId());
946946
assert sourceNode != null && sourceNode.containsShard(index, shardRouting);
947947
RoutingNode routingNode = sourceNode.getRoutingNode();
948-
Decision canRemain = allocation.deciders().canRemain(shardRouting, routingNode, allocation);
949-
if (canRemain.type() != Decision.Type.NO && canRemain.type() != Decision.Type.NOT_PREFERRED) {
950-
return MoveDecision.createRemainYesDecision(canRemain);
948+
Decision canRemainDecision = allocation.deciders().canRemain(shardRouting, routingNode, allocation);
949+
if (canRemainDecision.type() != Decision.Type.NO && canRemainDecision.type() != Decision.Type.NOT_PREFERRED) {
950+
return MoveDecision.createRemainYesDecision(canRemainDecision);
951951
}
952952

953953
// Check predicate to decide whether to assess movement options
954-
if (canRemain.type() == Type.NOT_PREFERRED && nonPreferredPredicate.test(shardRouting) == false) {
954+
if (canRemainDecision.type() == Type.NOT_PREFERRED && nonPreferredPredicate.test(shardRouting) == false) {
955955
return MoveDecision.NOT_TAKEN;
956956
}
957957

@@ -962,11 +962,11 @@ private MoveDecision decideMove(ProjectIndex index, ShardRouting shardRouting, P
962962
* This is not guaranteed to be balanced after this operation we still try best effort to
963963
* allocate on the minimal eligible node.
964964
*/
965-
final MoveDecision moveDecision = decideMove(sorter, shardRouting, sourceNode, canRemain, this::decideCanAllocate);
965+
final MoveDecision moveDecision = decideMove(sorter, shardRouting, sourceNode, canRemainDecision, this::decideCanAllocate);
966966
if (moveDecision.cannotRemainAndCannotMove()) {
967967
final boolean shardsOnReplacedNode = allocation.metadata().nodeShutdowns().contains(shardRouting.currentNodeId(), REPLACE);
968968
if (shardsOnReplacedNode) {
969-
return decideMove(sorter, shardRouting, sourceNode, canRemain, this::decideCanForceAllocateForVacate);
969+
return decideMove(sorter, shardRouting, sourceNode, canRemainDecision, this::decideCanForceAllocateForVacate);
970970
}
971971
}
972972
return moveDecision;

x-pack/plugin/shutdown/src/main/java/org/elasticsearch/xpack/shutdown/TransportGetShutdownStatusAction.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -285,9 +285,9 @@ static ShutdownShardMigrationStatus shardMigrationStatus(
285285
.map(shardRouting -> new Tuple<>(shardRouting, allocationService.explainShardAllocation(shardRouting, allocation)))
286286
// Given that we're checking the status of a node that's shutting down, no shards should be allowed to remain
287287
.filter(pair -> {
288-
assert pair.v2().getMoveDecision().canRemain() == false
288+
assert pair.v2().getMoveDecision().cannotRemain()
289289
: "shard [" + pair + "] can remain on node [" + nodeId + "], but that node is shutting down";
290-
return pair.v2().getMoveDecision().canRemain() == false;
290+
return pair.v2().getMoveDecision().cannotRemain();
291291
})
292292
// These shards will move as soon as possible
293293
.filter(pair -> pair.v2().getMoveDecision().getAllocationDecision().equals(AllocationDecision.YES) == false)

0 commit comments

Comments
 (0)