You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lucene.apache.org by ab...@apache.org on 2020/07/01 15:29:47 UTC
[lucene-solr] 03/11: SOLR-12847: Remove maxShardsPerNode, step 2.
This is an automated email from the ASF dual-hosted git repository.
ab pushed a commit to branch jira/solr-12847-2
in repository https://gitbox.apache.org/repos/asf/lucene-solr.git
commit 4b691ecac24cd700bd7a008235b757d70cd09a4d
Author: Andrzej Bialecki <ab...@apache.org>
AuthorDate: Thu Apr 16 09:50:23 2020 +0200
SOLR-12847: Remove maxShardsPerNode, step 2.
---
.../solr/cloud/api/collections/AddReplicaCmd.java | 11 ++--
.../solr/handler/admin/CollectionsHandler.java | 4 +-
.../apache/solr/cloud/CollectionsAPISolrJTest.java | 7 ---
.../collections/CollectionTooManyReplicasTest.java | 59 ++++++++++++++--------
.../cloud/api/collections/TestCollectionAPI.java | 25 ---------
.../test/org/apache/solr/cloud/rule/RulesTest.java | 2 +-
.../solrj/request/CollectionAdminRequest.java | 10 ++++
.../solr/common/params/CollectionAdminParams.java | 5 ++
8 files changed, 60 insertions(+), 63 deletions(-)
diff --git a/solr/core/src/java/org/apache/solr/cloud/api/collections/AddReplicaCmd.java b/solr/core/src/java/org/apache/solr/cloud/api/collections/AddReplicaCmd.java
index 8c6f002..b88867c 100644
--- a/solr/core/src/java/org/apache/solr/cloud/api/collections/AddReplicaCmd.java
+++ b/solr/core/src/java/org/apache/solr/cloud/api/collections/AddReplicaCmd.java
@@ -62,6 +62,7 @@ import org.apache.solr.common.cloud.ReplicaPosition;
import org.apache.solr.common.cloud.Slice;
import org.apache.solr.common.cloud.ZkNodeProps;
import org.apache.solr.common.cloud.ZkStateReader;
+import org.apache.solr.common.params.CollectionAdminParams;
import org.apache.solr.common.params.CommonAdminParams;
import org.apache.solr.common.params.CoreAdminParams;
import org.apache.solr.common.params.ModifiableSolrParams;
@@ -76,12 +77,6 @@ import org.slf4j.LoggerFactory;
public class AddReplicaCmd implements OverseerCollectionMessageHandler.Cmd {
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
- /**
- * When AddReplica is called with this set to true, then we do not try to find node assignments
- * for the add replica API. If set to true, a valid "node" should be specified.
- */
- public static final String SKIP_NODE_ASSIGNMENT = "skipNodeAssignment";
-
private final OverseerCollectionMessageHandler ocmh;
public AddReplicaCmd(OverseerCollectionMessageHandler ocmh) {
@@ -230,7 +225,7 @@ public class AddReplicaCmd implements OverseerCollectionMessageHandler.Cmd {
ZkStateReader.SHARD_ID_PROP, withCollectionShard,
"node", createReplica.node,
// since we already computed node assignments (which include assigning a node for this withCollection replica) we want to skip the assignment step
- SKIP_NODE_ASSIGNMENT, "true",
+ CollectionAdminParams.SKIP_NODE_ASSIGNMENT, "true",
CommonAdminParams.WAIT_FOR_FINAL_STATE, Boolean.TRUE.toString()); // set to true because we want `withCollection` to be ready after this collection is created
addReplica(clusterState, props, results, null);
}
@@ -346,7 +341,7 @@ public class AddReplicaCmd implements OverseerCollectionMessageHandler.Cmd {
EnumMap<Replica.Type, Integer> replicaTypeVsCount,
AtomicReference< PolicyHelper.SessionWrapper> sessionWrapper) throws IOException, InterruptedException {
boolean skipCreateReplicaInClusterState = message.getBool(SKIP_CREATE_REPLICA_IN_CLUSTER_STATE, false);
- boolean skipNodeAssignment = message.getBool(SKIP_NODE_ASSIGNMENT, false);
+ boolean skipNodeAssignment = message.getBool(CollectionAdminParams.SKIP_NODE_ASSIGNMENT, false);
String sliceName = message.getStr(SHARD_ID_PROP);
DocCollection collection = clusterState.getCollection(collectionName);
diff --git a/solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java b/solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java
index 7593d4a..13ecc27 100644
--- a/solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java
+++ b/solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java
@@ -144,6 +144,7 @@ import static org.apache.solr.common.params.CollectionAdminParams.COUNT_PROP;
import static org.apache.solr.common.params.CollectionAdminParams.FOLLOW_ALIASES;
import static org.apache.solr.common.params.CollectionAdminParams.PROPERTY_NAME;
import static org.apache.solr.common.params.CollectionAdminParams.PROPERTY_VALUE;
+import static org.apache.solr.common.params.CollectionAdminParams.SKIP_NODE_ASSIGNMENT;
import static org.apache.solr.common.params.CollectionAdminParams.WITH_COLLECTION;
import static org.apache.solr.common.params.CollectionParams.CollectionAction.*;
import static org.apache.solr.common.params.CommonAdminParams.ASYNC;
@@ -937,7 +938,8 @@ public class CollectionsHandler extends RequestHandlerBase implements Permission
TLOG_REPLICAS,
PULL_REPLICAS,
CREATE_NODE_SET,
- FOLLOW_ALIASES);
+ FOLLOW_ALIASES,
+ SKIP_NODE_ASSIGNMENT);
return copyPropertiesWithPrefix(req.getParams(), props, COLL_PROP_PREFIX);
}),
OVERSEERSTATUS_OP(OVERSEERSTATUS, (req, rsp, h) -> new LinkedHashMap<>()),
diff --git a/solr/core/src/test/org/apache/solr/cloud/CollectionsAPISolrJTest.java b/solr/core/src/test/org/apache/solr/cloud/CollectionsAPISolrJTest.java
index c4f9bdd..94290e7 100644
--- a/solr/core/src/test/org/apache/solr/cloud/CollectionsAPISolrJTest.java
+++ b/solr/core/src/test/org/apache/solr/cloud/CollectionsAPISolrJTest.java
@@ -1075,13 +1075,6 @@ public class CollectionsAPISolrJTest extends SolrCloudTestCase {
waitForState("Expecting attribute 'replicationFactor' to be 25", collection,
(n, c) -> 25 == c.getReplicationFactor());
- CollectionAdminRequest.modifyCollection(collection, null)
- .unsetAttribute("maxShardsPerNode")
- .process(cluster.getSolrClient());
-
- waitForState("Expecting attribute 'maxShardsPerNode' to be deleted", collection,
- (n, c) -> null == c.get("maxShardsPerNode"));
-
expectThrows(IllegalArgumentException.class,
"An attempt to set unknown collection attribute should have failed",
() -> CollectionAdminRequest.modifyCollection(collection, null)
diff --git a/solr/core/src/test/org/apache/solr/cloud/api/collections/CollectionTooManyReplicasTest.java b/solr/core/src/test/org/apache/solr/cloud/api/collections/CollectionTooManyReplicasTest.java
index 95dfc4b..6d2ea05 100644
--- a/solr/core/src/test/org/apache/solr/cloud/api/collections/CollectionTooManyReplicasTest.java
+++ b/solr/core/src/test/org/apache/solr/cloud/api/collections/CollectionTooManyReplicasTest.java
@@ -21,8 +21,10 @@ import java.util.Map;
import java.util.stream.Collectors;
import org.apache.lucene.util.LuceneTestCase.Slow;
+import org.apache.solr.client.solrj.SolrRequest;
import org.apache.solr.client.solrj.embedded.JettySolrRunner;
import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.cloud.CloudTestUtils;
import org.apache.solr.cloud.SolrCloudTestCase;
import org.apache.solr.common.cloud.DocCollection;
import org.apache.solr.common.cloud.Replica;
@@ -49,6 +51,7 @@ public class CollectionTooManyReplicasTest extends SolrCloudTestCase {
@Test
public void testAddTooManyReplicas() throws Exception {
+
final String collectionName = "TooManyReplicasInSeveralFlavors";
CollectionAdminRequest.createCollection(collectionName, "conf", 2, 1)
.process(cluster.getSolrClient());
@@ -60,24 +63,33 @@ public class CollectionTooManyReplicasTest extends SolrCloudTestCase {
// Get a node to use for the "node" parameter.
String nodeName = getAllNodeNames(collectionName).get(0);
- // Add a replica using the "node" parameter (no "too many replicas check")
+ // Add a replica using the "node" parameter
+ // AND force skipping the node assignment (no "too many replicas check")
// this node should have 2 replicas on it
CollectionAdminRequest.addReplicaToShard(collectionName, "shard1")
.setNode(nodeName)
+ //.setSkipNodeAssignment(true)
.process(cluster.getSolrClient());
- // Three replicas so far, should be able to create another one "normally"
- CollectionAdminRequest.addReplicaToShard(collectionName, "shard1")
- .process(cluster.getSolrClient());
+ // equivalent to maxShardsPerNode=1
+ String commands = "{ set-cluster-policy: [ {replica: '<2', shard: '#ANY', node: '#ANY', strict: true} ] }";
+ cluster.getSolrClient().request(CloudTestUtils.AutoScalingRequest.create(SolrRequest.METHOD.POST, commands));
- // This one should fail though, no "node" parameter specified
- Exception e = expectThrows(Exception.class, () -> {
+ for (int i = 0; i < 10; i++) {
+ // Three replicas so far, should be able to create another one "normally"
CollectionAdminRequest.addReplicaToShard(collectionName, "shard1")
.process(cluster.getSolrClient());
- });
+ }
+ CollectionAdminRequest.addReplicaToShard(collectionName, "shard1")
+ .process(cluster.getSolrClient());
+ // This one should fail though, no "node" parameter specified
+// Exception e = expectThrows(Exception.class, () -> {
+// CollectionAdminRequest.addReplicaToShard(collectionName, "shard1")
+// .process(cluster.getSolrClient());
+// });
- assertTrue("Should have gotten the right error message back",
- e.getMessage().contains("given the current number of eligible live nodes"));
+// assertTrue("Should have gotten the right error message back",
+// e.getMessage().contains("given the current number of eligible live nodes"));
// Oddly, we should succeed next just because setting property.name will not check for nodes being "full up"
@@ -118,6 +130,9 @@ public class CollectionTooManyReplicasTest extends SolrCloudTestCase {
@Test
public void testAddShard() throws Exception {
+ // equivalent to maxShardsPerNode=2
+ String commands = "{ set-cluster-policy: [ {replica: '<3', shard: '#ANY', node: '#ANY', strict: true} ] }";
+ cluster.getSolrClient().request(CloudTestUtils.AutoScalingRequest.create(SolrRequest.METHOD.POST, commands));
String collectionName = "TooManyReplicasWhenAddingShards";
CollectionAdminRequest.createCollectionWithImplicitRouter(collectionName, "conf", "shardstart", 2)
@@ -138,38 +153,40 @@ public class CollectionTooManyReplicasTest extends SolrCloudTestCase {
.process(cluster.getSolrClient());
});
assertTrue("Should have gotten the right error message back",
- e.getMessage().contains("given the current number of eligible live nodes"));
+ e.getMessage().contains("No node can satisfy the rules"));
// Hmmm, providing a nodeset also overrides the checks for max replicas, so prove it.
List<String> nodes = getAllNodeNames(collectionName);
- CollectionAdminRequest.createShard(collectionName, "shard4")
- .setNodeSet(String.join(",", nodes))
- .process(cluster.getSolrClient());
-
- // And just for yucks, insure we fail the "regular" one again.
Exception e2 = expectThrows(Exception.class, () -> {
+ CollectionAdminRequest.createShard(collectionName, "shard4")
+ .setNodeSet(String.join(",", nodes))
+ .process(cluster.getSolrClient());
+ });
+ assertTrue("Should have gotten the right error message back",
+ e2.getMessage().contains("No node can satisfy the rules"));
+
+// // And just for yucks, insure we fail the "regular" one again.
+ Exception e3 = expectThrows(Exception.class, () -> {
CollectionAdminRequest.createShard(collectionName, "shard5")
.process(cluster.getSolrClient());
});
assertTrue("Should have gotten the right error message back",
- e2.getMessage().contains("given the current number of eligible live nodes"));
+ e3.getMessage().contains("No node can satisfy the rules"));
// And finally, ensure that there are all the replicas we expect. We should have shards 1, 2 and 4 and each
// should have exactly two replicas
- waitForState("Expected shards shardstart, 1, 2 and 4, each with two active replicas", collectionName, (n, c) -> {
- return DocCollection.isFullyActive(n, c, 4, 2);
+ waitForState("Expected shards shardstart, 1, 2, each with two active replicas", collectionName, (n, c) -> {
+ return DocCollection.isFullyActive(n, c, 3, 2);
});
Map<String, Slice> slices = getCollectionState(collectionName).getSlicesMap();
- assertEquals("There should be exaclty four slices", slices.size(), 4);
+ assertEquals("There should be exaclty three slices", slices.size(), 3);
assertNotNull("shardstart should exist", slices.get("shardstart"));
assertNotNull("shard1 should exist", slices.get("shard1"));
assertNotNull("shard2 should exist", slices.get("shard2"));
- assertNotNull("shard4 should exist", slices.get("shard4"));
assertEquals("Shardstart should have exactly 2 replicas", 2, slices.get("shardstart").getReplicas().size());
assertEquals("Shard1 should have exactly 2 replicas", 2, slices.get("shard1").getReplicas().size());
assertEquals("Shard2 should have exactly 2 replicas", 2, slices.get("shard2").getReplicas().size());
- assertEquals("Shard4 should have exactly 2 replicas", 2, slices.get("shard4").getReplicas().size());
}
diff --git a/solr/core/src/test/org/apache/solr/cloud/api/collections/TestCollectionAPI.java b/solr/core/src/test/org/apache/solr/cloud/api/collections/TestCollectionAPI.java
index 291f0e8..96e32f9 100644
--- a/solr/core/src/test/org/apache/solr/cloud/api/collections/TestCollectionAPI.java
+++ b/solr/core/src/test/org/apache/solr/cloud/api/collections/TestCollectionAPI.java
@@ -95,7 +95,6 @@ public class TestCollectionAPI extends ReplicaPropertiesBase {
clusterStatusZNodeVersion();
testClusterStateMigration();
testCollectionCreationCollectionNameValidation();
- testCollectionCreationTooManyShards();
testReplicationFactorValidaton();
testCollectionCreationShardNameValidation();
testAliasCreationNameValidation();
@@ -104,30 +103,6 @@ public class TestCollectionAPI extends ReplicaPropertiesBase {
testModifyCollection(); // deletes replicationFactor property from collections, be careful adding new tests after this one!
}
- private void testCollectionCreationTooManyShards() throws Exception {
- try (CloudSolrClient client = createCloudClient(null)) {
- ModifiableSolrParams params = new ModifiableSolrParams();
- params.set("action", CollectionParams.CollectionAction.CREATE.toString());
- params.set("name", "collection_too_many");
- params.set("router.name", "implicit");
- params.set("numShards", "10");
- params.set("maxShardsPerNode", 1);
- params.set("shards", "b0,b1,b2,b3,b4,b5,b6,b7,b8,b9");
- SolrRequest request = new QueryRequest(params);
- request.setPath("/admin/collections");
-
- try {
- client.request(request);
- fail("A collection creation request with too many shards than allowed by maxShardsPerNode should not have succeeded");
- } catch (BaseHttpSolrClient.RemoteSolrException e) {
- final String errorMessage = e.getMessage();
- assertTrue(errorMessage.contains("Cannot create collection"));
- assertTrue(errorMessage.contains("This requires 10 shards to be created (higher than the allowed number)"));
- assertMissingCollection(client, "collection_too_many");
- }
- }
- }
-
private void assertMissingCollection(CloudSolrClient client, String collectionName) throws Exception {
ClusterState clusterState = client.getZkStateReader().getClusterState();
assertNull(clusterState.getCollectionOrNull(collectionName));
diff --git a/solr/core/src/test/org/apache/solr/cloud/rule/RulesTest.java b/solr/core/src/test/org/apache/solr/cloud/rule/RulesTest.java
index 17dd019..45a9cde 100644
--- a/solr/core/src/test/org/apache/solr/cloud/rule/RulesTest.java
+++ b/solr/core/src/test/org/apache/solr/cloud/rule/RulesTest.java
@@ -153,7 +153,7 @@ public class RulesTest extends SolrCloudTestCase {
// adding an additional replica should fail since our rule says at most one replica
// per node, and we know every node already has one replica
expectedException.expect(BaseHttpSolrClient.RemoteSolrException.class);
- expectedException.expectMessage(containsString("current number of eligible live nodes 0"));
+ expectedException.expectMessage(containsString("Could not identify nodes matching the rules"));
CollectionAdminRequest.addReplicaToShard(rulesColl, "shard2").process(cluster.getSolrClient());
}
diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java b/solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java
index be419fb..981f8d3 100644
--- a/solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java
+++ b/solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java
@@ -71,6 +71,7 @@ import static org.apache.solr.common.params.CollectionAdminParams.COUNT_PROP;
import static org.apache.solr.common.params.CollectionAdminParams.CREATE_NODE_SET_PARAM;
import static org.apache.solr.common.params.CollectionAdminParams.CREATE_NODE_SET_SHUFFLE_PARAM;
import static org.apache.solr.common.params.CollectionAdminParams.ROUTER_PREFIX;
+import static org.apache.solr.common.params.CollectionAdminParams.SKIP_NODE_ASSIGNMENT;
import static org.apache.solr.common.params.CollectionAdminParams.WITH_COLLECTION;
/**
@@ -2099,6 +2100,7 @@ public abstract class CollectionAdminRequest<T extends CollectionAdminResponse>
protected Properties properties;
protected Replica.Type type;
protected Integer nrtReplicas, tlogReplicas, pullReplicas;
+ protected Boolean skipNodeAssignment;
protected String createNodeSet;
private AddReplica(String collection, String shard, String routeKey, Replica.Type type) {
@@ -2134,6 +2136,11 @@ public abstract class CollectionAdminRequest<T extends CollectionAdminResponse>
return this;
}
+ public AddReplica setSkipNodeAssignment(Boolean skipNodeAssignment) {
+ this.skipNodeAssignment = skipNodeAssignment;
+ return this;
+ }
+
public String getRouteKey() {
return routeKey;
}
@@ -2229,6 +2236,9 @@ public abstract class CollectionAdminRequest<T extends CollectionAdminResponse>
if (node != null) {
params.add(CoreAdminParams.NODE, node);
}
+ if (skipNodeAssignment != null) {
+ params.add(SKIP_NODE_ASSIGNMENT, String.valueOf(skipNodeAssignment));
+ }
if (instanceDir != null) {
params.add(CoreAdminParams.INSTANCE_DIR, instanceDir);
}
diff --git a/solr/solrj/src/java/org/apache/solr/common/params/CollectionAdminParams.java b/solr/solrj/src/java/org/apache/solr/common/params/CollectionAdminParams.java
index 5af2345..5b6af2e 100644
--- a/solr/solrj/src/java/org/apache/solr/common/params/CollectionAdminParams.java
+++ b/solr/solrj/src/java/org/apache/solr/common/params/CollectionAdminParams.java
@@ -127,4 +127,9 @@ public interface CollectionAdminParams {
/** Option to follow aliases when deciding the target of a collection admin command. */
String FOLLOW_ALIASES = "followAliases";
+ /**
+ * When AddReplica is called with this set to true, then we do not try to find node assignments
+ * for the add replica API. If set to true, a valid "node" should be specified.
+ */
+ String SKIP_NODE_ASSIGNMENT = "skipNodeAssignment";
}