diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index cbc8d19b4aa..39065e2826c 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -72,6 +72,8 @@ Bug Fixes * SOLR-13199: Return proper error when invalid parentFilter is passed in ChildDocTransformer (Johannes Kloos, Munendra S N, David Smiley, Mikhail Khludnev) +* SOLR-14347: Autoscaling placement wrong when concurrent replica placements are calculated. (ab) + Other Changes --------------------- * SOLR-14197: SolrResourceLoader: marked many methods as deprecated, and in some cases rerouted exiting logic to avoid diff --git a/solr/core/src/java/org/apache/solr/cloud/autoscaling/sim/SimCloudManager.java b/solr/core/src/java/org/apache/solr/cloud/autoscaling/sim/SimCloudManager.java index 3bcc2730999..b2bd5d5b4f3 100644 --- a/solr/core/src/java/org/apache/solr/cloud/autoscaling/sim/SimCloudManager.java +++ b/solr/core/src/java/org/apache/solr/cloud/autoscaling/sim/SimCloudManager.java @@ -315,7 +315,7 @@ public class SimCloudManager implements SolrCloudManager { config = cloudManager.getDistribStateManager().getAutoScalingConfig(); } Set nodeTags = new HashSet<>(SimUtils.COMMON_NODE_TAGS); - nodeTags.addAll(config.getPolicy().getParams()); + nodeTags.addAll(config.getPolicy().getParamNames()); Set replicaTags = new HashSet<>(SimUtils.COMMON_REPLICA_TAGS); replicaTags.addAll(config.getPolicy().getPerReplicaAttributes()); cloudManager.getSimClusterStateProvider().copyFrom(other.getClusterStateProvider()); diff --git a/solr/core/src/java/org/apache/solr/cloud/autoscaling/sim/SnapshotNodeStateProvider.java b/solr/core/src/java/org/apache/solr/cloud/autoscaling/sim/SnapshotNodeStateProvider.java index 5a49635ab65..8ccf8491bf5 100644 --- a/solr/core/src/java/org/apache/solr/cloud/autoscaling/sim/SnapshotNodeStateProvider.java +++ b/solr/core/src/java/org/apache/solr/cloud/autoscaling/sim/SnapshotNodeStateProvider.java @@ -53,7 +53,7 @@ public class SnapshotNodeStateProvider implements NodeStateProvider { config = other.getDistribStateManager().getAutoScalingConfig(); } Set nodeTags = new HashSet<>(SimUtils.COMMON_NODE_TAGS); - nodeTags.addAll(config.getPolicy().getParams()); + nodeTags.addAll(config.getPolicy().getParamNames()); Set replicaTags = new HashSet<>(SimUtils.COMMON_REPLICA_TAGS); replicaTags.addAll(config.getPolicy().getPerReplicaAttributes()); for (String node : other.getClusterStateProvider().getLiveNodes()) { diff --git a/solr/core/src/test/org/apache/solr/cloud/api/collections/ConcurrentCreateCollectionTest.java b/solr/core/src/test/org/apache/solr/cloud/api/collections/ConcurrentCreateCollectionTest.java index b864ce28a23..9c1b4e9c4e0 100644 --- a/solr/core/src/test/org/apache/solr/cloud/api/collections/ConcurrentCreateCollectionTest.java +++ b/solr/core/src/test/org/apache/solr/cloud/api/collections/ConcurrentCreateCollectionTest.java @@ -36,12 +36,10 @@ import org.apache.solr.common.cloud.Slice; import org.junit.After; import org.junit.Before; import org.junit.BeforeClass; -import org.junit.Ignore; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -@Ignore("SOLR-13884") public class ConcurrentCreateCollectionTest extends SolrCloudTestCase { private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); @@ -51,8 +49,8 @@ public class ConcurrentCreateCollectionTest extends SolrCloudTestCase { @BeforeClass public static void setupCluster() throws Exception { configureCluster(NODES) - // .addConfig("conf", configset("cloud-minimal")) - .addConfig("conf", configset("_default")) + .addConfig("conf", configset("cloud-minimal")) + //.addConfig("conf", configset("_default")) .configure(); } diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/MoveReplicaSuggester.java b/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/MoveReplicaSuggester.java index 45f18b1519a..d529922da39 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/MoveReplicaSuggester.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/MoveReplicaSuggester.java @@ -72,8 +72,8 @@ public class MoveReplicaSuggester extends Suggester { int result = -1; if (!force && srcRowModified.isLive && targetRow.isLive) { - result = tmpSession.getPolicy().clusterPreferences.get(0).compare(srcRowModified, tmpSession.getNode(targetRow.node), true); - if (result == 0) result = tmpSession.getPolicy().clusterPreferences.get(0).compare(srcRowModified, tmpSession.getNode(targetRow.node), false); + result = tmpSession.getPolicy().getClusterPreferences().get(0).compare(srcRowModified, tmpSession.getNode(targetRow.node), true); + if (result == 0) result = tmpSession.getPolicy().getClusterPreferences().get(0).compare(srcRowModified, tmpSession.getNode(targetRow.node), false); } if (result <= 0) { diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/Policy.java b/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/Policy.java index afe04b51ba7..39237dd5392 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/Policy.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/Policy.java @@ -91,21 +91,21 @@ public class Policy implements MapWriter { */ private static final List DEFAULT_PARAMS_OF_INTEREST = Arrays.asList(ImplicitSnitch.DISK, ImplicitSnitch.CORES); - final Map> policies; - final List clusterPolicy; - final List clusterPreferences; - final List> params; - final List perReplicaAttributes; - final int zkVersion; + private final Map> policies; + private final List clusterPolicy; + private final List clusterPreferences; + private final List> params; + private final List perReplicaAttributes; + private final int zkVersion; /** * True if cluster policy, preferences and custom policies are all non-existent */ - final boolean empty; + private final boolean empty; /** * True if cluster preferences was originally empty, false otherwise. It is used to figure out if * the current preferences were implicitly added or not. */ - final boolean emptyPreferences; + private final boolean emptyPreferences; public Policy() { this(Collections.emptyMap()); @@ -215,11 +215,11 @@ public class Policy implements MapWriter { } public List getClusterPolicy() { - return clusterPolicy; + return Collections.unmodifiableList(clusterPolicy); } public List getClusterPreferences() { - return clusterPreferences; + return Collections.unmodifiableList(clusterPreferences); } @Override @@ -355,7 +355,11 @@ public class Policy implements MapWriter { } public Session createSession(SolrCloudManager cloudManager) { - return createSession(cloudManager, null); + return new Session(cloudManager, this, null); + } + + public Session createSession(SolrCloudManager cloudManager, Transaction tx) { + return new Session(cloudManager, this, tx); } public enum SortParam { @@ -392,10 +396,6 @@ public class Policy implements MapWriter { } } - private Session createSession(SolrCloudManager cloudManager, Transaction tx) { - return new Session(cloudManager, tx); - } - public static List mergePolicies(String coll, List collPolicy, List globalPolicy) { @@ -462,7 +462,11 @@ public class Policy implements MapWriter { return policies; } - public List getParams() { + public List> getParams() { + return Collections.unmodifiableList(params); + } + + public List getParamNames() { return params.stream().map(Pair::first).collect(toList()); } @@ -470,6 +474,10 @@ public class Policy implements MapWriter { return Collections.unmodifiableList(perReplicaAttributes); } + public int getZkVersion() { + return zkVersion; + } + /** * Compares two {@link Row} loads according to a policy. * @@ -503,33 +511,22 @@ public class Policy implements MapWriter { * a cluster state. * */ - public class Session implements MapWriter { + public static class Session implements MapWriter { final List nodes; final SolrCloudManager cloudManager; final List matrix; final NodeStateProvider nodeStateProvider; final int znodeVersion; Set collections = new HashSet<>(); + final Policy policy; List expandedClauses; List violations = new ArrayList<>(); Transaction transaction; - private Session(List nodes, SolrCloudManager cloudManager, - List matrix, List expandedClauses, int znodeVersion, - NodeStateProvider nodeStateProvider, Transaction transaction) { - this.transaction = transaction; - this.nodes = nodes; - this.cloudManager = cloudManager; - this.matrix = matrix; - this.expandedClauses = expandedClauses; - this.znodeVersion = znodeVersion; - this.nodeStateProvider = nodeStateProvider; - for (Row row : matrix) row.session = this; - } - - Session(SolrCloudManager cloudManager, Transaction transaction) { + Session(SolrCloudManager cloudManager, Policy policy, Transaction transaction) { this.transaction = transaction; + this.policy = policy; ClusterState state = null; this.nodeStateProvider = cloudManager.getNodeStateProvider(); try { @@ -545,7 +542,7 @@ public class Policy implements MapWriter { collections.addAll(nodeStateProvider.getReplicaInfo(node, Collections.emptyList()).keySet()); } - expandedClauses = clusterPolicy.stream() + expandedClauses = policy.getClusterPolicy().stream() .filter(clause -> !clause.isPerCollectiontag()) .collect(Collectors.toList()); @@ -566,29 +563,48 @@ public class Policy implements MapWriter { ClusterStateProvider stateProvider = cloudManager.getClusterStateProvider(); for (String c : collections) { - addClausesForCollection(stateProvider, c); + addClausesForCollection(policy, expandedClauses, stateProvider, c); } Collections.sort(expandedClauses); matrix = new ArrayList<>(nodes.size()); - for (String node : nodes) matrix.add(new Row(node, params, perReplicaAttributes, this)); + for (String node : nodes) matrix.add(new Row(node, policy.getParams(), policy.getPerReplicaAttributes(), this)); applyRules(); } - void addClausesForCollection(ClusterStateProvider stateProvider, String c) { + private Session(List nodes, SolrCloudManager cloudManager, + List matrix, List expandedClauses, int znodeVersion, + NodeStateProvider nodeStateProvider, Policy policy, Transaction transaction) { + this.transaction = transaction; + this.policy = policy; + this.nodes = nodes; + this.cloudManager = cloudManager; + this.matrix = matrix; + this.expandedClauses = expandedClauses; + this.znodeVersion = znodeVersion; + this.nodeStateProvider = nodeStateProvider; + for (Row row : matrix) row.session = this; + } + + + void addClausesForCollection(ClusterStateProvider stateProvider, String collection) { + addClausesForCollection(policy, expandedClauses, stateProvider, collection); + } + + public static void addClausesForCollection(Policy policy, List clauses, ClusterStateProvider stateProvider, String c) { String p = stateProvider.getPolicyNameByCollection(c); if (p != null) { - List perCollPolicy = policies.get(p); + List perCollPolicy = policy.getPolicies().get(p); if (perCollPolicy == null) { return; } } - expandedClauses.addAll(mergePolicies(c, policies.getOrDefault(p, emptyList()), clusterPolicy)); + clauses.addAll(mergePolicies(c, policy.getPolicies().getOrDefault(p, emptyList()), policy.getClusterPolicy())); } Session copy() { - return new Session(nodes, cloudManager, getMatrixCopy(), expandedClauses, znodeVersion, nodeStateProvider, transaction); + return new Session(nodes, cloudManager, getMatrixCopy(), expandedClauses, znodeVersion, nodeStateProvider, policy, transaction); } public Row getNode(String node) { @@ -603,7 +619,7 @@ public class Policy implements MapWriter { } public Policy getPolicy() { - return Policy.this; + return policy; } @@ -620,7 +636,7 @@ public class Policy implements MapWriter { } void sortNodes() { - setApproxValuesAndSortNodes(clusterPreferences, matrix); + setApproxValuesAndSortNodes(policy.getClusterPreferences(), matrix); } diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/PolicyHelper.java b/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/PolicyHelper.java index 955295a3b3c..71870118db8 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/PolicyHelper.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/PolicyHelper.java @@ -121,7 +121,7 @@ public class PolicyHelper { policyMapping.set(optionalPolicyMapping); SessionWrapper sessionWrapper = null; - Policy.Session session = null; + Policy.Session origSession = null; try { try { SESSION_WRAPPPER_REF.set(sessionWrapper = getSession(delegatingManager)); @@ -129,7 +129,9 @@ public class PolicyHelper { throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "unable to get autoscaling policy session", e); } - session = sessionWrapper.session; + origSession = sessionWrapper.session; + // new session needs to be created to avoid side-effects from per-collection policies + Policy.Session session = new Policy.Session(delegatingManager, origSession.policy, origSession.transaction); Map diskSpaceReqd = new HashMap<>(); try { DocCollection coll = cloudManager.getClusterStateProvider().getCollection(collName); @@ -193,7 +195,7 @@ public class PolicyHelper { } finally { policyMapping.remove(); if (sessionWrapper != null) { - sessionWrapper.returnSession(session); + sessionWrapper.returnSession(origSession); } } return positions; @@ -563,7 +565,7 @@ public class PolicyHelper { this.ref = ref; this.zkVersion = session == null ? 0 : - session.getPolicy().zkVersion; + session.getPolicy().getZkVersion(); } public Policy.Session get() { diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/Suggester.java b/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/Suggester.java index 22da717442b..26f1a9d640f 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/Suggester.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/Suggester.java @@ -177,7 +177,7 @@ public abstract class Suggester implements MapWriter { // the source node is dead so live nodes may not have it for (String srcNode : srcNodes) { if (session.matrix.stream().noneMatch(row -> row.node.equals(srcNode))) { - session.matrix.add(new Row(srcNode, session.getPolicy().params, session.getPolicy().perReplicaAttributes, session)); + session.matrix.add(new Row(srcNode, session.getPolicy().getParams(), session.getPolicy().getPerReplicaAttributes(), session)); } } } @@ -324,7 +324,7 @@ public abstract class Suggester implements MapWriter { List testChangedMatrix(boolean executeInStrictMode, Policy.Session session) { if (this.deviations != null) this.lastBestDeviation = this.deviations; this.deviations = null; - Policy.setApproxValuesAndSortNodes(session.getPolicy().clusterPreferences, session.matrix); + Policy.setApproxValuesAndSortNodes(session.getPolicy().getClusterPreferences(), session.matrix); List errors = new ArrayList<>(); for (Clause clause : session.expandedClauses) { Clause originalClause = clause.derivedFrom == null ? clause : clause.derivedFrom; diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/cloud/autoscaling/TestPolicy.java b/solr/solrj/src/test/org/apache/solr/client/solrj/cloud/autoscaling/TestPolicy.java index b785142336e..35c969c7f00 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/cloud/autoscaling/TestPolicy.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/cloud/autoscaling/TestPolicy.java @@ -2757,7 +2757,7 @@ public class TestPolicy extends SolrTestCaseJ4 { if (!row.isLive) deadNodes++; } - Policy.setApproxValuesAndSortNodes(policy.clusterPreferences, rows); + Policy.setApproxValuesAndSortNodes(policy.getClusterPreferences(), rows); for (int i = 0; i < deadNodes; i++) { assertFalse(rows.get(i).isLive);