Skip to content

Commit 76c2b99

Browse files
nicktindallnicktindall
committed
Use compareTo-like interface for sorting decisions/nodes
1 parent 0434558 commit 76c2b99

4 files changed

Lines changed: 44 additions & 40 deletions

File tree

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -530,12 +530,12 @@ private MoveDecision explainRebalanceDecision(final ProjectIndex index, final Sh
530530
// if the simulated weight delta with the shard moved away is better than the weight delta
531531
// with the shard remaining on the current node, and we are allowed to allocate to the
532532
// node in question, then allow the rebalance
533-
if (rebalanceConditionsMet && canAllocate.type().isBetterAcrossNodes(bestRebalanceCanAllocateDecisionType)) {
533+
if (rebalanceConditionsMet && canAllocate.type().compareToBetweenNodes(bestRebalanceCanAllocateDecisionType) > 0) {
534534
// Overwrite the best decision since it is better than the last. This means that YES/THROTTLE decisions will replace
535535
// NOT_PREFERRED/NO decisions, and a YES decision will replace a THROTTLE decision. NOT_PREFERRED will also replace
536536
// NO, even if neither are acted upon for rebalancing, for allocation explain purposes.
537537
bestRebalanceCanAllocateDecisionType = canAllocate.type();
538-
if (canAllocate.type().isBetterAcrossNodes(Type.NOT_PREFERRED)) {
538+
if (canAllocate.type().compareToBetweenNodes(Type.NOT_PREFERRED) > 0) {
539539
// Movement is only allowed to THROTTLE/YES nodes. NOT_PREFERRED is the same as no for rebalancing, since
540540
// rebalancing aims to distribute resource usage and NOT_PREFERRED means the move could cause hot-spots.
541541
targetNode = node;
@@ -1053,7 +1053,7 @@ private MoveDecision decideMove(
10531053
// Relocating a shard from one NOT_PREFERRED node to another NOT_PREFERRED node would not improve the situation.
10541054
continue;
10551055
}
1056-
if (allocationDecision.type().isBetterAcrossNodes(bestDecision)) {
1056+
if (allocationDecision.type().compareToBetweenNodes(bestDecision) > 0) {
10571057
bestDecision = allocationDecision.type();
10581058
if (bestDecision == Type.YES) {
10591059
targetNode = target;

‎server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/AllocationDeciders.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,7 @@ private Decision withDeciders(
208208
Decision mostNegativeDecision = Decision.YES;
209209
for (AllocationDecider decider : deciders) {
210210
var decision = deciderAction.apply(decider);
211-
if (decision.type().isWorseForTheSameNode(mostNegativeDecision.type())) {
211+
if (decision.type().compareToBetweenDecisions(mostNegativeDecision.type()) < 0) {
212212
mostNegativeDecision = decision;
213213
if (mostNegativeDecision.type() == Decision.Type.NO) {
214214
traceNoDecisions(decider, decision, logMessageCreator);

‎server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/Decision.java‎

Lines changed: 30 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -117,17 +117,23 @@ private static Single readSingleFrom(StreamInput in) throws IOException {
117117
* This enumeration defines the possible types of decisions
118118
*/
119119
enum Type implements Writeable {
120-
// ordered by positiveness; order matters for serialization and comparison
121-
NO,
122-
NOT_PREFERRED,
123-
// Temporarily throttled is a better choice than choosing a not-preferred node,
124-
// but NOT_PREFERRED and THROTTLED are generally not comparable.
125-
THROTTLE,
126-
YES;
120+
// order matters only for serialization, do NOT use for comparison
121+
NO(0, 0),
122+
NOT_PREFERRED(1, 2),
123+
THROTTLE(2, 1),
124+
YES(3, 3);
127125

128126
// visible for testing
129127
static final TransportVersion ALLOCATION_DECISION_NOT_PREFERRED = TransportVersion.fromName("allocation_decision_not_preferred");
130128

129+
private final int nodeComparisonOrdinal;
130+
private final int decisionComparisonOrdinal;
131+
132+
Type(int nodeComparisonOrdinal, int decisionComparisonOrdinal) {
133+
this.nodeComparisonOrdinal = nodeComparisonOrdinal;
134+
this.decisionComparisonOrdinal = decisionComparisonOrdinal;
135+
}
136+
131137
public static Type readFrom(StreamInput in) throws IOException {
132138
if (in.getTransportVersion().supports(AllocationDecision.ADD_NOT_PREFERRED_ALLOCATION_DECISION)) {
133139
return in.readEnum(Type.class);
@@ -173,38 +179,29 @@ public void writeTo(StreamOutput out) throws IOException {
173179
}
174180

175181
/**
176-
* Compares this decision against a new decision. A temporarily THROTTLE'ed node is a better choice than a NOT_PREFERRED node.
182+
* Compares this decision against another decision (for choosing a node)
177183
* <p>
178-
* This is used in simulation, which does not encounter THROTTLE; and allocation explain, which will choose a target node that is
179-
* THROTTLED temporarily (which simulation ignores and will assign to) over one that is NOT_PREFERRED. Simulation
180-
* and explain both use the same code paths in the Allocator code, so this helper is used in the Allocator code.
184+
* This comparison is used at the node level when deciding which node to allocate a shard to. We prefer to wait and allocate a shard
185+
* to a <code>THROTTLE</code>'d node than to move a shard to a <code>NOT_PREFERRED</code> node immediately.
186+
*
187+
* @return 0 when this == other, 1 when this &gt; other, -1 when this &lt; other
181188
*/
182-
public boolean isBetterAcrossNodes(Type newDecision) {
183-
return this.compareTo(newDecision) > 0;
189+
public int compareToBetweenNodes(Type other) {
190+
return Integer.compare(nodeComparisonOrdinal, other.nodeComparisonOrdinal);
184191
}
185192

186193
/**
187-
* Compares this decision against a new decision. THROTTLE is considered a worse decision than NOT_PREFERRED.
194+
* Compares this decision against another decision (for decision-aggregation)
188195
* <p>
189-
* A node that has both NOT_PREFERRED and THROTTLE decider results must surface THROTTLE so that the shard does not get moved.
190-
* THROTTLE will go away eventually and NOT_PREFERRED will surface, and then different action can be taken. This is important for
191-
* the Reconciler executing real shard movement decisions. Allocation explain will also use it for individual node explanations.
196+
* This comparison is used when aggregating the results from many deciders. If one decider returns <code>THROTTLE</code> and
197+
* another returns <code>NOT_PREFERRED</code>, we want to return <code>THROTTLE</code> to ensure we respect any throttling deciders.
198+
* This can only occur in the reconciler or non-desired balancer, in both cases if we see a <code>THROTTLE</code> we want to
199+
* respect that until it resolves.
200+
*
201+
* @return 0 when this == other, 1 when this &gt; other, -1 when this &lt; other
192202
*/
193-
public boolean isWorseForTheSameNode(Type decision) {
194-
return switch (decision) {
195-
case NO -> {
196-
yield false;
197-
}
198-
case NOT_PREFERRED -> {
199-
yield (this == YES) ? false /* this=YES is not worse than NOT_PREFERRED */ : true /* this=THROTTLE is worse */;
200-
}
201-
case THROTTLE -> {
202-
yield (this == NO) ? true : false /* all Types other than NO are better than THROTTLE */ ;
203-
}
204-
case YES -> {
205-
yield true;
206-
}
207-
};
203+
public int compareToBetweenDecisions(Type other) {
204+
return Integer.compare(decisionComparisonOrdinal, other.decisionComparisonOrdinal);
208205
}
209206

210207
/**
@@ -314,7 +311,7 @@ public Type type() {
314311
Decision.Type worst = Type.YES;
315312
for (Single decision : decisions) {
316313
final var next = decision.type();
317-
if (next.isWorseForTheSameNode(worst)) {
314+
if (next.compareToBetweenDecisions(worst) < 0) {
318315
worst = next;
319316
}
320317
}

‎server/src/test/java/org/elasticsearch/cluster/routing/allocation/decider/DecisionTests.java‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,15 @@
1717
import org.elasticsearch.test.EnumSerializationTestUtils;
1818

1919
import java.io.IOException;
20+
import java.util.Arrays;
2021
import java.util.List;
2122

2223
import static org.elasticsearch.cluster.routing.allocation.decider.Decision.Type.ALLOCATION_DECISION_NOT_PREFERRED;
2324
import static org.elasticsearch.cluster.routing.allocation.decider.Decision.Type.NO;
2425
import static org.elasticsearch.cluster.routing.allocation.decider.Decision.Type.NOT_PREFERRED;
2526
import static org.elasticsearch.cluster.routing.allocation.decider.Decision.Type.THROTTLE;
2627
import static org.elasticsearch.cluster.routing.allocation.decider.Decision.Type.YES;
28+
import static org.hamcrest.Matchers.equalTo;
2729

2830
/**
2931
* A class for unit testing the {@link Decision} class.
@@ -53,9 +55,14 @@ public void testTypeEnumOrder() {
5355
EnumSerializationTestUtils.assertEnumSerialization(Type.class, NO, NOT_PREFERRED, THROTTLE, YES);
5456
}
5557

56-
public void testTypeHigherThan() {
57-
assertTrue(
58-
YES.isBetterAcrossNodes(THROTTLE) && THROTTLE.isBetterAcrossNodes(NOT_PREFERRED) && NOT_PREFERRED.isBetterAcrossNodes(NO)
58+
public void testTypeComparisonOrder() {
59+
assertThat(
60+
shuffledList(Arrays.asList(Type.values())).stream().sorted(Type::compareToBetweenDecisions).toList(),
61+
equalTo(List.of(NO, THROTTLE, NOT_PREFERRED, YES))
62+
);
63+
assertThat(
64+
shuffledList(Arrays.asList(Type.values())).stream().sorted(Type::compareToBetweenNodes).toList(),
65+
equalTo(List.of(NO, NOT_PREFERRED, THROTTLE, YES))
5966
);
6067
}
6168

0 commit comments

Comments
 (0)