Fix debug mode in MaxRetryAllocationDecider (#89973)

In #62275 we refactored this code a bit and inadvertently reversed the sense of this conditional when running in debug mode. This commit fixes the mistake.
elastic · Sep 9, 2022 · cb40ab8 · cb40ab8
1 parent 85058a7
commit cb40ab8
Show file tree

Hide file tree

Showing 3 changed files with 23 additions and 22 deletions.
diff --git a/docs/changelog/89973.yaml b/docs/changelog/89973.yaml
@@ -0,0 +1,5 @@
+pr: 89973
+summary: Fix debug mode in `MaxRetryAllocationDecider`
+area: Allocation
+type: bug
+issues: []
diff --git a/.../java/org/elasticsearch/cluster/routing/allocation/decider/MaxRetryAllocationDecider.java b/.../java/org/elasticsearch/cluster/routing/allocation/decider/MaxRetryAllocationDecider.java
@@ -61,7 +61,7 @@ private static Decision decisionWithFailures(
     }
 
     private static Decision debugDecision(Decision decision, UnassignedInfo unassignedInfo, int numFailedAllocations, int maxRetry) {
-        if (decision.type() == Decision.Type.YES) {
+        if (decision.type() == Decision.Type.NO) {
             return Decision.single(
                 Decision.Type.NO,
                 NAME,

diff --git a/...est/java/org/elasticsearch/cluster/routing/allocation/MaxRetryAllocationDeciderTests.java b/...est/java/org/elasticsearch/cluster/routing/allocation/MaxRetryAllocationDeciderTests.java
@@ -198,12 +198,8 @@ public void testFailedAllocation() {
             assertThat(unassignedPrimary.unassignedInfo().getMessage(), containsString("boom" + i));
             // MaxRetryAllocationDecider#canForceAllocatePrimary should return YES decisions because canAllocate returns YES here
             assertEquals(
-                Decision.YES,
-                new MaxRetryAllocationDecider().canForceAllocatePrimary(
-                    unassignedPrimary,
-                    null,
-                    new RoutingAllocation(null, null, clusterState, null, null, 0)
-                )
+                Decision.Type.YES,
+                new MaxRetryAllocationDecider().canForceAllocatePrimary(unassignedPrimary, null, newRoutingAllocation(clusterState)).type()
             );
         }
         // now we go and check that we are actually stick to unassigned on the next failure
@@ -227,12 +223,8 @@ public void testFailedAllocation() {
             assertThat(unassignedPrimary.unassignedInfo().getMessage(), containsString("boom"));
             // MaxRetryAllocationDecider#canForceAllocatePrimary should return a NO decision because canAllocate returns NO here
             assertEquals(
-                Decision.NO,
-                new MaxRetryAllocationDecider().canForceAllocatePrimary(
-                    unassignedPrimary,
-                    null,
-                    new RoutingAllocation(null, null, clusterState, null, null, 0)
-                )
+                Decision.Type.NO,
+                new MaxRetryAllocationDecider().canForceAllocatePrimary(unassignedPrimary, null, newRoutingAllocation(clusterState)).type()
             );
         }
 
@@ -267,12 +259,12 @@ public void testFailedAllocation() {
         assertThat(unassignedPrimary.unassignedInfo().getMessage(), containsString("boom"));
         // bumped up the max retry count, so canForceAllocatePrimary should return a YES decision
         assertEquals(
-            Decision.YES,
+            Decision.Type.YES,
             new MaxRetryAllocationDecider().canForceAllocatePrimary(
                 routingTable.index("idx").shard(0).shards().get(0),
                 null,
-                new RoutingAllocation(null, null, clusterState, null, null, 0)
-            )
+                newRoutingAllocation(clusterState)
+            ).type()
         );
 
         // now we start the shard
@@ -304,13 +296,17 @@ public void testFailedAllocation() {
         assertThat(unassignedPrimary.unassignedInfo().getMessage(), containsString("ZOOOMG"));
         // Counter reset, so MaxRetryAllocationDecider#canForceAllocatePrimary should return a YES decision
         assertEquals(
-            Decision.YES,
-            new MaxRetryAllocationDecider().canForceAllocatePrimary(
-                unassignedPrimary,
-                null,
-                new RoutingAllocation(null, null, clusterState, null, null, 0)
-            )
+            Decision.Type.YES,
+            new MaxRetryAllocationDecider().canForceAllocatePrimary(unassignedPrimary, null, newRoutingAllocation(clusterState)).type()
         );
     }
 
+    private RoutingAllocation newRoutingAllocation(ClusterState clusterState) {
+        final RoutingAllocation routingAllocation = new RoutingAllocation(null, null, clusterState, null, null, 0);
+        if (randomBoolean()) {
+            routingAllocation.setDebugMode(randomFrom(RoutingAllocation.DebugMode.values()));
+        }
+        return routingAllocation;
+    }
+
 }