From 04e7ef1cbb4e762b7277593c3671d3b758bb64d9 Mon Sep 17 00:00:00 2001 From: Dianna Hohensee Date: Thu, 30 Oct 2025 13:18:27 -0700 Subject: [PATCH 1/2] Tighten assertions in MoveDecision static method helpers --- .../routing/allocation/MoveDecision.java | 6 +++--- .../routing/allocation/MoveDecisionTests.java | 17 +++++++++-------- 2 files changed, 12 insertions(+), 11 deletions(-) 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 index 384b08561d8b0..d7f8ffb0015bf 100644 --- a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/MoveDecision.java +++ b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/MoveDecision.java @@ -93,8 +93,8 @@ public void writeTo(StreamOutput out) throws IOException { * be forced to move to another node. */ public static MoveDecision createRemainYesDecision(Decision canRemainDecision) { - assert canRemainDecision.type() != Type.NO; - assert canRemainDecision.type() != Type.NOT_PREFERRED; + assert canRemainDecision != null; + assert canRemainDecision.type() == Type.YES : "create decision with MoveDecision#move instead"; if (canRemainDecision == Decision.YES) { return CACHED_STAY_DECISION; } @@ -117,7 +117,7 @@ public static MoveDecision move( @Nullable List nodeDecisions ) { assert canRemainDecision != null; - assert canRemainDecision.type() != Type.YES : "create decision with MoveDecision#stay instead"; + assert canRemainDecision.type() != Type.YES : "create decision with MoveDecision#createRemainYesDecision instead"; if (nodeDecisions == null && moveDecision == AllocationDecision.NO) { // the final decision is NO (no node to move the shard to) and we are not in explain mode, return a cached version return CACHED_CANNOT_MOVE_DECISION; 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 index 379c2881bfcc2..1a12eb42c7d87 100644 --- a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/MoveDecisionTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/MoveDecisionTests.java @@ -56,6 +56,10 @@ public void testCachedDecisions() { assertEquals(stay1.getExplanation(), stay2.getExplanation()); } + public void testMoveDecisionThrowsOnYes() { + assertThrows(AssertionError.class, () -> MoveDecision.move(Decision.YES, AllocationDecision.NO, null, null)); + } + public void testStayDecision() { MoveDecision stay = MoveDecision.createRemainYesDecision(Decision.YES); assertTrue(stay.canRemain()); @@ -64,18 +68,15 @@ public void testStayDecision() { assertNull(stay.getNodeDecisions()); assertEquals(AllocationDecision.NO_ATTEMPT, stay.getAllocationDecision()); - stay = MoveDecision.createRemainYesDecision(Decision.YES); - assertTrue(stay.canRemain()); - assertFalse(stay.cannotRemainAndCanMove()); - assertTrue(stay.isDecisionTaken()); - assertNull(stay.getNodeDecisions()); - assertEquals(AllocationDecision.NO_ATTEMPT, stay.getAllocationDecision()); + assertThrows(AssertionError.class, () -> MoveDecision.createRemainYesDecision(Decision.NO)); + assertThrows(AssertionError.class, () -> MoveDecision.createRemainYesDecision(Decision.NOT_PREFERRED)); + assertThrows(AssertionError.class, () -> MoveDecision.createRemainYesDecision(Decision.THROTTLE)); } public void testDecisionWithNodeExplanations() { DiscoveryNode node1 = DiscoveryNodeUtils.builder("node1").roles(emptySet()).build(); DiscoveryNode node2 = DiscoveryNodeUtils.builder("node2").roles(emptySet()).build(); - Decision nodeDecision = randomFrom(Decision.NO, Decision.THROTTLE, Decision.YES); + Decision nodeDecision = randomFrom(Decision.NO, Decision.THROTTLE, Decision.YES, Decision.NOT_PREFERRED); List nodeDecisions = new ArrayList<>(); nodeDecisions.add(new NodeAllocationResult(node1, nodeDecision, 2)); nodeDecisions.add(new NodeAllocationResult(node2, nodeDecision, 1)); @@ -101,7 +102,7 @@ public void testSerialization() throws IOException { nodeDecisions.add( new NodeAllocationResult( node2, - finalDecision.allowed() ? Decision.YES : randomFrom(Decision.NO, Decision.THROTTLE, Decision.YES), + finalDecision.allowed() ? Decision.YES : randomFrom(Decision.NO, Decision.THROTTLE, Decision.YES, Decision.NOT_PREFERRED), 1 ) ); From ee86469154e15c0133f727894b6c64d7fa03e4b6 Mon Sep 17 00:00:00 2001 From: Dianna Hohensee Date: Thu, 30 Oct 2025 14:57:16 -0700 Subject: [PATCH 2/2] add debug to assertion --- .../cluster/routing/allocation/MoveDecision.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) 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 index d7f8ffb0015bf..4713ef78c1390 100644 --- a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/MoveDecision.java +++ b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/MoveDecision.java @@ -94,7 +94,8 @@ public void writeTo(StreamOutput out) throws IOException { */ public static MoveDecision createRemainYesDecision(Decision canRemainDecision) { assert canRemainDecision != null; - assert canRemainDecision.type() == Type.YES : "create decision with MoveDecision#move instead"; + assert canRemainDecision.type() == Type.YES + : "Create decision with MoveDecision#move instead. canRemain Decision: " + canRemainDecision; if (canRemainDecision == Decision.YES) { return CACHED_STAY_DECISION; } @@ -117,7 +118,8 @@ public static MoveDecision move( @Nullable List nodeDecisions ) { assert canRemainDecision != null; - assert canRemainDecision.type() != Type.YES : "create decision with MoveDecision#createRemainYesDecision instead"; + assert canRemainDecision.type() != Type.YES + : "Create decision with MoveDecision#createRemainYesDecision instead. canRemain Decision: " + canRemainDecision; if (nodeDecisions == null && moveDecision == AllocationDecision.NO) { // the final decision is NO (no node to move the shard to) and we are not in explain mode, return a cached version return CACHED_CANNOT_MOVE_DECISION;