Make AllocationDecider canRemain loop tighter (#86503)

No need to collect a multi decision here (which is surprisingly prominent in the profiling) when we're not using it in the end anyway.
elastic · May 6, 2022 · c4ce394 · c4ce394
1 parent 97222b7
commit c4ce394
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 20 deletions.
diff --git a/...rc/main/java/org/elasticsearch/cluster/routing/allocation/decider/AllocationDeciders.java b/...rc/main/java/org/elasticsearch/cluster/routing/allocation/decider/AllocationDeciders.java
@@ -86,29 +86,44 @@ public Decision canRemain(ShardRouting shardRouting, RoutingNode node, RoutingAl
             }
             return Decision.NO;
         }
-        Decision.Multi ret = new Decision.Multi();
-        for (AllocationDecider allocationDecider : allocations) {
-            Decision decision = allocationDecider.canRemain(shardRouting, node, allocation);
-            // short track if a NO is returned.
-            if (decision.type() == Decision.Type.NO) {
-                if (logger.isTraceEnabled()) {
-                    logger.trace(
-                        "Shard [{}] can not remain on node [{}] due to [{}]",
-                        shardRouting,
-                        node.nodeId(),
-                        allocationDecider.getClass().getSimpleName()
-                    );
-                }
-                if (allocation.debugDecision() == false) {
-                    return Decision.NO;
-                } else {
+        if (allocation.debugDecision()) {
+            Decision.Multi ret = new Decision.Multi();
+            for (AllocationDecider allocationDecider : allocations) {
+                Decision decision = allocationDecider.canRemain(shardRouting, node, allocation);
+                // short track if a NO is returned.
+                if (decision.type() == Decision.Type.NO) {
+                    maybeTraceLogNoDecision(shardRouting, node, allocationDecider);
                     ret.add(decision);
+                } else {
+                    addDecision(ret, decision, allocation);
                 }
-            } else {
-                addDecision(ret, decision, allocation);
             }
+            return ret;
+        } else {
+            // tighter loop if debug information is not collected: don't collect yes decisions + break out right away on NO
+            Decision ret = Decision.YES;
+            for (AllocationDecider allocationDecider : allocations) {
+                switch (allocationDecider.canRemain(shardRouting, node, allocation).type()) {
+                    case NO -> {
+                        maybeTraceLogNoDecision(shardRouting, node, allocationDecider);
+                        return Decision.NO;
+                    }
+                    case THROTTLE -> ret = Decision.THROTTLE;
+                }
+            }
+            return ret;
+        }
+    }
+
+    private void maybeTraceLogNoDecision(ShardRouting shardRouting, RoutingNode node, AllocationDecider allocationDecider) {
+        if (logger.isTraceEnabled()) {
+            logger.trace(
+                "Shard [{}] can not remain on node [{}] due to [{}]",
+                shardRouting,
+                node.nodeId(),
+                allocationDecider.getClass().getSimpleName()
+            );
         }
-        return ret;
     }
 
     public Decision canAllocate(IndexMetadata indexMetadata, RoutingNode node, RoutingAllocation allocation) {

diff --git a/...st/java/org/elasticsearch/cluster/routing/allocation/decider/AllocationDecidersTests.java b/...st/java/org/elasticsearch/cluster/routing/allocation/decider/AllocationDecidersTests.java
@@ -97,7 +97,12 @@ public Decision canRebalance(RoutingAllocation allocation) {
         verify(deciders.canAllocate(shardRouting, allocation), matcher);
         verify(deciders.canRebalance(shardRouting, allocation), matcher);
         verify(deciders.canRebalance(allocation), matcher);
-        verify(deciders.canRemain(shardRouting, routingNode, allocation), matcher);
+        final Decision canRemainResult = deciders.canRemain(shardRouting, routingNode, allocation);
+        if (allocation.debugDecision()) {
+            verify(canRemainResult, matcher);
+        } else {
+            assertSame(canRemainResult, Decision.YES);
+        }
         verify(deciders.canForceAllocatePrimary(shardRouting, routingNode, allocation), matcher);
         verify(deciders.shouldAutoExpandToNode(idx, null, allocation), matcher);
     }