diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 31dae0bc33b..fc880f5dbb2 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -89,6 +89,10 @@ Bug Fixes * SOLR-12449: Response /autoscaling/diagnostics shows improper json (noble) +* SOLR-11676: Keep nrtReplicas and replicationFactor in sync while creating a collection and modifying a collection + (Varun Thacker) + + Optimizations ---------------------- diff --git a/solr/core/src/java/org/apache/solr/cloud/overseer/CollectionMutator.java b/solr/core/src/java/org/apache/solr/cloud/overseer/CollectionMutator.java index 05ae0121b91..5ce537f96bb 100644 --- a/solr/core/src/java/org/apache/solr/cloud/overseer/CollectionMutator.java +++ b/solr/core/src/java/org/apache/solr/cloud/overseer/CollectionMutator.java @@ -38,6 +38,8 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import static org.apache.solr.common.cloud.ZkStateReader.COLLECTION_PROP; +import static org.apache.solr.common.cloud.ZkStateReader.NRT_REPLICAS; +import static org.apache.solr.common.cloud.ZkStateReader.REPLICATION_FACTOR; import static org.apache.solr.common.params.CommonParams.NAME; public class CollectionMutator { @@ -106,9 +108,12 @@ public class CollectionMutator { Map m = coll.shallowCopy(); boolean hasAnyOps = false; for (String prop : CollectionsHandler.MODIFIABLE_COLL_PROPS) { - if(message.get(prop)!= null) { + if (message.get(prop) != null) { hasAnyOps = true; m.put(prop,message.get(prop)); + if (prop == REPLICATION_FACTOR) { //SOLR-11676 : keep NRT_REPLICAS and REPLICATION_FACTOR in sync + m.put(NRT_REPLICAS, message.get(REPLICATION_FACTOR)); + } } } 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 269bb50641d..9c45a01810a 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 @@ -484,6 +484,22 @@ public class CollectionsHandler extends RequestHandlerBase implements Permission if (props.get(STATE_FORMAT) == null) { props.put(STATE_FORMAT, "2"); } + + if (props.get(REPLICATION_FACTOR) != null && props.get(NRT_REPLICAS) != null) { + //TODO: Remove this in 8.0 . Keep this for SolrJ client back-compat. See SOLR-11676 for more details + int replicationFactor = Integer.parseInt((String) props.get(REPLICATION_FACTOR)); + int nrtReplicas = Integer.parseInt((String) props.get(NRT_REPLICAS)); + if (replicationFactor != nrtReplicas) { + throw new SolrException(ErrorCode.BAD_REQUEST, + "Cannot specify both replicationFactor and nrtReplicas as they mean the same thing"); + } + } + if (props.get(REPLICATION_FACTOR) != null) { + props.put(NRT_REPLICAS, props.get(REPLICATION_FACTOR)); + } else if (props.get(NRT_REPLICAS) != null) { + props.put(REPLICATION_FACTOR, props.get(NRT_REPLICAS)); + } + addMapObject(props, RULE); addMapObject(props, SNITCH); verifyRuleParams(h.coreContainer, props); @@ -907,6 +923,9 @@ public class CollectionsHandler extends RequestHandlerBase implements Permission addMapObject(m, SNITCH); for (String prop : MODIFIABLE_COLL_PROPS) DocCollection.verifyProp(m, prop); verifyRuleParams(h.coreContainer, m); + if (m.get(REPLICATION_FACTOR) != null) { + m.put(NRT_REPLICAS, m.get(REPLICATION_FACTOR)); + } return m; }), MIGRATESTATEFORMAT_OP(MIGRATESTATEFORMAT, (req, rsp, h) -> copy(req.getParams().required(), null, COLLECTION_PROP)), 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 d9ea73c08da..40047f74a29 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 @@ -92,11 +92,72 @@ public class TestCollectionAPI extends ReplicaPropertiesBase { clusterStatusZNodeVersion(); testClusterStateMigration(); testCollectionCreationCollectionNameValidation(); + testReplicationFactorValidaton(); testCollectionCreationShardNameValidation(); testAliasCreationNameValidation(); testShardCreationNameValidation(); } + private void testReplicationFactorValidaton() throws Exception { + try (CloudSolrClient client = createCloudClient(null)) { + //Test that you can't specify both replicationFactor and nrtReplicas + ModifiableSolrParams params = new ModifiableSolrParams(); + params.set("action", CollectionParams.CollectionAction.CREATE.toString()); + params.set("name", "test_repFactorColl"); + params.set("numShards", "1"); + params.set("replicationFactor", "1"); + params.set("nrtReplicas", "2"); + SolrRequest request = new QueryRequest(params); + request.setPath("/admin/collections"); + + try { + client.request(request); + fail(); + } catch (RemoteSolrException e) { + final String errorMessage = e.getMessage(); + assertTrue(errorMessage.contains("Cannot specify both replicationFactor and nrtReplicas as they mean the same thing")); + } + + //Create it again correctly + CollectionAdminRequest.Create req = CollectionAdminRequest.createCollection("test_repFactorColl", "conf1", 1, 3, 0, 0); + client.request(req); + + waitForCollection(cloudClient.getZkStateReader(), "test_repFactorColl", 1); + waitForRecoveriesToFinish("test_repFactorColl", false); + + //Assert that replicationFactor has also been set to 3 + assertCountsForRepFactorAndNrtReplicas(client, "test_repFactorColl"); + + params = new ModifiableSolrParams(); + params.set("action", CollectionParams.CollectionAction.MODIFYCOLLECTION.toString()); + params.set("collection", "test_repFactorColl"); + params.set("replicationFactor", "4"); + request = new QueryRequest(params); + request.setPath("/admin/collections"); + client.request(request); + + assertCountsForRepFactorAndNrtReplicas(client, "test_repFactorColl"); + } + } + + private void assertCountsForRepFactorAndNrtReplicas(CloudSolrClient client, String collectionName) throws Exception { + ModifiableSolrParams params = new ModifiableSolrParams(); + params.set("action", CollectionParams.CollectionAction.CLUSTERSTATUS.toString()); + params.set("collection", collectionName); + QueryRequest request = new QueryRequest(params); + request.setPath("/admin/collections"); + + NamedList rsp = client.request(request); + NamedList cluster = (NamedList) rsp.get("cluster"); + assertNotNull("Cluster state should not be null", cluster); + NamedList collections = (NamedList) cluster.get("collections"); + assertNotNull("Collections should not be null in cluster state", collections); + assertEquals(1, collections.size()); + Map collection = (Map) collections.get(collectionName); + assertNotNull(collection); + assertEquals(collection.get("replicationFactor"), collection.get("nrtReplicas")); + } + private void clusterStatusWithCollectionAndShard() throws IOException, SolrServerException { try (CloudSolrClient client = createCloudClient(null)) { diff --git a/solr/core/src/test/org/apache/solr/handler/admin/TestCollectionAPIs.java b/solr/core/src/test/org/apache/solr/handler/admin/TestCollectionAPIs.java index 0ac81a86b8c..aa3bac6b63b 100644 --- a/solr/core/src/test/org/apache/solr/handler/admin/TestCollectionAPIs.java +++ b/solr/core/src/test/org/apache/solr/handler/admin/TestCollectionAPIs.java @@ -88,20 +88,20 @@ public class TestCollectionAPIs extends SolrTestCaseJ4 { //test a simple create collection call compareOutput(apiBag, "/collections", POST, "{create:{name:'newcoll', config:'schemaless', numShards:2, replicationFactor:2 }}", null, - "{name:newcoll, fromApi:'true', replicationFactor:'2', collection.configName:schemaless, numShards:'2', stateFormat:'2', operation:create}"); + "{name:newcoll, fromApi:'true', replicationFactor:'2', nrtReplicas:'2', collection.configName:schemaless, numShards:'2', stateFormat:'2', operation:create}"); compareOutput(apiBag, "/collections", POST, "{create:{name:'newcoll', config:'schemaless', numShards:2, nrtReplicas:2 }}", null, - "{name:newcoll, fromApi:'true', nrtReplicas:'2', collection.configName:schemaless, numShards:'2', stateFormat:'2', operation:create}"); + "{name:newcoll, fromApi:'true', nrtReplicas:'2', replicationFactor:'2', collection.configName:schemaless, numShards:'2', stateFormat:'2', operation:create}"); compareOutput(apiBag, "/collections", POST, "{create:{name:'newcoll', config:'schemaless', numShards:2, nrtReplicas:2, tlogReplicas:2, pullReplicas:2 }}", null, - "{name:newcoll, fromApi:'true', nrtReplicas:'2', tlogReplicas:'2', pullReplicas:'2', collection.configName:schemaless, numShards:'2', stateFormat:'2', operation:create}"); + "{name:newcoll, fromApi:'true', nrtReplicas:'2', replicationFactor:'2', tlogReplicas:'2', pullReplicas:'2', collection.configName:schemaless, numShards:'2', stateFormat:'2', operation:create}"); //test a create collection with custom properties compareOutput(apiBag, "/collections", POST, "{create:{name:'newcoll', config:'schemaless', numShards:2, replicationFactor:2, properties:{prop1:'prop1val', prop2: prop2val} }}", null, - "{name:newcoll, fromApi:'true', replicationFactor:'2', collection.configName:schemaless, numShards:'2', stateFormat:'2', operation:create, property.prop1:prop1val, property.prop2:prop2val}"); + "{name:newcoll, fromApi:'true', replicationFactor:'2', nrtReplicas:'2', collection.configName:schemaless, numShards:'2', stateFormat:'2', operation:create, property.prop1:prop1val, property.prop2:prop2val}"); compareOutput(apiBag, "/collections", POST, 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 4566033cfef..e3e9196a55e 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 @@ -508,7 +508,6 @@ public abstract class CollectionAdminRequest params.set("router.field", routerField); } if (nrtReplicas != null) { - params.set( ZkStateReader.REPLICATION_FACTOR, nrtReplicas);// Keep both for compatibility? params.set( ZkStateReader.NRT_REPLICAS, nrtReplicas); } if (autoAddReplicas != null) {