From 9d40277d4cb92982f6e29768f60216bf541962c2 Mon Sep 17 00:00:00 2001 From: Henning Andersen <33268011+henningandersen@users.noreply.github.com> Date: Wed, 19 Feb 2020 11:12:50 +0100 Subject: [PATCH] Deciders should not by default collect yes'es (#52438) AllocationDeciders would collect Yes decisions when not asking for debug info. Changed to only include Yes decisions when debug is requested (explain). --- .../decider/AllocationDeciders.java | 57 ++++---- .../decider/AllocationDecidersTests.java | 126 ++++++++++++++++++ 2 files changed, 152 insertions(+), 31 deletions(-) create mode 100644 server/src/test/java/org/elasticsearch/cluster/routing/allocation/decider/AllocationDecidersTests.java diff --git a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/AllocationDeciders.java b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/AllocationDeciders.java index f7ec43b5197..3f0607df3ca 100644 --- a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/AllocationDeciders.java +++ b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/AllocationDeciders.java @@ -30,8 +30,6 @@ import org.elasticsearch.cluster.routing.allocation.RoutingAllocation; import java.util.Collection; import java.util.Collections; -import static org.elasticsearch.cluster.routing.allocation.RoutingAllocation.DebugMode.EXCLUDE_YES_DECISIONS; - /** * A composite {@link AllocationDecider} combining the "decision" of multiple * {@link AllocationDecider} implementations into a single allocation decision. @@ -58,9 +56,8 @@ public class AllocationDeciders extends AllocationDecider { } else { ret.add(decision); } - } else if (decision != Decision.ALWAYS - && (allocation.getDebugMode() != EXCLUDE_YES_DECISIONS || decision.type() != Decision.Type.YES)) { - ret.add(decision); + } else { + addDecision(ret, decision, allocation); } } return ret; @@ -86,11 +83,8 @@ public class AllocationDeciders extends AllocationDecider { } else { ret.add(decision); } - } else if (decision != Decision.ALWAYS - && (allocation.getDebugMode() != EXCLUDE_YES_DECISIONS || decision.type() != Decision.Type.YES)) { - // the assumption is that a decider that returns the static instance Decision#ALWAYS - // does not really implements canAllocate - ret.add(decision); + } else { + addDecision(ret, decision, allocation); } } return ret; @@ -118,9 +112,8 @@ public class AllocationDeciders extends AllocationDecider { } else { ret.add(decision); } - } else if (decision != Decision.ALWAYS - && (allocation.getDebugMode() != EXCLUDE_YES_DECISIONS || decision.type() != Decision.Type.YES)) { - ret.add(decision); + } else { + addDecision(ret, decision, allocation); } } return ret; @@ -138,9 +131,8 @@ public class AllocationDeciders extends AllocationDecider { } else { ret.add(decision); } - } else if (decision != Decision.ALWAYS - && (allocation.getDebugMode() != EXCLUDE_YES_DECISIONS || decision.type() != Decision.Type.YES)) { - ret.add(decision); + } else { + addDecision(ret, decision, allocation); } } return ret; @@ -158,9 +150,8 @@ public class AllocationDeciders extends AllocationDecider { } else { ret.add(decision); } - } else if (decision != Decision.ALWAYS - && (allocation.getDebugMode() != EXCLUDE_YES_DECISIONS || decision.type() != Decision.Type.YES)) { - ret.add(decision); + } else { + addDecision(ret, decision, allocation); } } return ret; @@ -178,9 +169,8 @@ public class AllocationDeciders extends AllocationDecider { } else { ret.add(decision); } - } else if (decision != Decision.ALWAYS - && (allocation.getDebugMode() != EXCLUDE_YES_DECISIONS || decision.type() != Decision.Type.YES)) { - ret.add(decision); + } else { + addDecision(ret, decision, allocation); } } return ret; @@ -198,9 +188,8 @@ public class AllocationDeciders extends AllocationDecider { } else { ret.add(decision); } - } else if (decision != Decision.ALWAYS - && (allocation.getDebugMode() != EXCLUDE_YES_DECISIONS || decision.type() != Decision.Type.YES)) { - ret.add(decision); + } else { + addDecision(ret, decision, allocation); } } return ret; @@ -218,9 +207,8 @@ public class AllocationDeciders extends AllocationDecider { } else { ret.add(decision); } - } else if (decision != Decision.ALWAYS - && (allocation.getDebugMode() != EXCLUDE_YES_DECISIONS || decision.type() != Decision.Type.YES)) { - ret.add(decision); + } else { + addDecision(ret, decision, allocation); } } return ret; @@ -247,11 +235,18 @@ public class AllocationDeciders extends AllocationDecider { } else { ret.add(decision); } - } else if (decision != Decision.ALWAYS - && (allocation.getDebugMode() != EXCLUDE_YES_DECISIONS || decision.type() != Decision.Type.YES)) { - ret.add(decision); + } else { + addDecision(ret, decision, allocation); } } return ret; } + + private void addDecision(Decision.Multi ret, Decision decision, RoutingAllocation allocation) { + // We never add ALWAYS decisions and only add YES decisions when requested by debug mode (since Multi default is YES). + if (decision != Decision.ALWAYS + && (allocation.getDebugMode() == RoutingAllocation.DebugMode.ON || decision.type() != Decision.Type.YES)) { + ret.add(decision); + } + } } diff --git a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/decider/AllocationDecidersTests.java b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/decider/AllocationDecidersTests.java new file mode 100644 index 00000000000..85b1498b2f8 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/decider/AllocationDecidersTests.java @@ -0,0 +1,126 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.cluster.routing.allocation.decider; + +import org.elasticsearch.Version; +import org.elasticsearch.cluster.ClusterName; +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.metadata.IndexMetaData; +import org.elasticsearch.cluster.node.DiscoveryNode; +import org.elasticsearch.cluster.routing.RecoverySource; +import org.elasticsearch.cluster.routing.RoutingNode; +import org.elasticsearch.cluster.routing.ShardRouting; +import org.elasticsearch.cluster.routing.UnassignedInfo; +import org.elasticsearch.cluster.routing.allocation.RoutingAllocation; +import org.elasticsearch.index.shard.ShardId; +import org.elasticsearch.test.ESTestCase; +import org.hamcrest.Matcher; +import org.hamcrest.Matchers; + +import java.util.Collection; +import java.util.Collections; + +public class AllocationDecidersTests extends ESTestCase { + + public void testDebugMode() { + verifyDebugMode(RoutingAllocation.DebugMode.ON, Matchers.hasSize(1)); + } + + public void testNoDebugMode() { + verifyDebugMode(RoutingAllocation.DebugMode.OFF, Matchers.empty()); + } + + public void testDebugExcludeYesMode() { + verifyDebugMode(RoutingAllocation.DebugMode.EXCLUDE_YES_DECISIONS, Matchers.empty()); + } + + private void verifyDebugMode(RoutingAllocation.DebugMode mode, Matcher> matcher) { + AllocationDeciders deciders = new AllocationDeciders(Collections.singleton(new AllocationDecider() { + @Override + public Decision canAllocate(ShardRouting shardRouting, RoutingNode node, RoutingAllocation allocation) { + return Decision.YES; + } + + @Override + public Decision canRebalance(ShardRouting shardRouting, RoutingAllocation allocation) { + return Decision.YES; + } + + @Override + public Decision canRemain(ShardRouting shardRouting, RoutingNode node, RoutingAllocation allocation) { + return Decision.YES; + } + + @Override + public Decision canAllocate(ShardRouting shardRouting, RoutingAllocation allocation) { + return Decision.YES; + } + + @Override + public Decision canAllocate(IndexMetaData indexMetaData, RoutingNode node, RoutingAllocation allocation) { + return Decision.YES; + } + + @Override + public Decision canAllocate(RoutingNode node, RoutingAllocation allocation) { + return Decision.YES; + } + + @Override + public Decision shouldAutoExpandToNode(IndexMetaData indexMetaData, DiscoveryNode node, RoutingAllocation allocation) { + return Decision.YES; + } + + @Override + public Decision canRebalance(RoutingAllocation allocation) { + return Decision.YES; + } + })); + + ClusterState clusterState = ClusterState.builder(new ClusterName("test")).build(); + final RoutingAllocation allocation = new RoutingAllocation(deciders, + clusterState.getRoutingNodes(), clusterState, null, 0L); + + allocation.setDebugMode(mode); + final UnassignedInfo unassignedInfo = new UnassignedInfo(UnassignedInfo.Reason.INDEX_CREATED, "_message"); + final ShardRouting shardRouting = ShardRouting.newUnassigned(new ShardId("test", "testUUID", 0), true, + RecoverySource.ExistingStoreRecoverySource.INSTANCE, unassignedInfo); + IndexMetaData idx = + IndexMetaData.builder("idx").settings(settings(Version.CURRENT)).numberOfShards(1).numberOfReplicas(0).build(); + + RoutingNode routingNode = new RoutingNode("testNode", null); + verify(deciders.canAllocate(shardRouting, routingNode, allocation), matcher); + verify(deciders.canAllocate(idx, routingNode, allocation), matcher); + verify(deciders.canAllocate(shardRouting, allocation), matcher); + verify(deciders.canAllocate(routingNode, allocation), matcher); + verify(deciders.canRebalance(shardRouting, allocation), matcher); + verify(deciders.canRebalance(allocation), matcher); + verify(deciders.canRemain(shardRouting, routingNode, allocation), matcher); + verify(deciders.canForceAllocatePrimary(shardRouting, routingNode, allocation), matcher); + verify(deciders.shouldAutoExpandToNode(idx, null, allocation), matcher); + } + + private void verify(Decision decision, Matcher> matcher) { + assertThat(decision.type(), Matchers.equalTo(Decision.Type.YES)); + assertThat(decision, Matchers.instanceOf(Decision.Multi.class)); + Decision.Multi multi = (Decision.Multi) decision; + assertThat(multi.getDecisions(), matcher); + } +}