diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index a72e982c8a1..35b5b1154d0 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -225,6 +225,9 @@ Bug Fixes * SOLR-12248, SOLR-4647: Grouping is broken on docValues-only fields (non-stored, non-indexed) (Erick Ericsson, Adrien Grand, Munendra S N, Scott Stults, Ishan Chattopadhyaya) +* SOLR-5970: Return correct status upon collection creation failure (Abraham Elmahrek, Ishan Chattopadhyaya, + Jason Gerlowski, Kesharee Nandan Vishwakarma) + Improvements ---------------------- diff --git a/solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java b/solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java index 500aae06761..dbbaa26c304 100644 --- a/solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java +++ b/solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java @@ -301,7 +301,7 @@ public class CreateCollectionCmd implements OverseerCollectionMessageHandler.Cmd // element, which may be interpreted by the user as a positive ack ocmh.cleanupCollection(collectionName, new NamedList()); log.info("Cleaned up artifacts for failed create collection for [{}]", collectionName); - return; + throw new SolrException(ErrorCode.BAD_REQUEST, "Underlying core creation failed while creating collection: " + collectionName); } else { log.debug("Finished create command on all shards for collection: {}", collectionName); diff --git a/solr/core/src/test/org/apache/solr/cloud/CreateCollectionCleanupTest.java b/solr/core/src/test/org/apache/solr/cloud/CreateCollectionCleanupTest.java index c569581bede..733a4a13932 100644 --- a/solr/core/src/test/org/apache/solr/cloud/CreateCollectionCleanupTest.java +++ b/solr/core/src/test/org/apache/solr/cloud/CreateCollectionCleanupTest.java @@ -23,6 +23,7 @@ import static org.hamcrest.CoreMatchers.not; import java.util.Properties; import org.apache.solr.client.solrj.impl.CloudSolrClient; +import org.apache.solr.client.solrj.impl.HttpSolrClient; import org.apache.solr.client.solrj.request.CollectionAdminRequest; import org.apache.solr.client.solrj.response.CollectionAdminResponse; import org.apache.solr.client.solrj.response.RequestStatusState; @@ -78,8 +79,9 @@ public class CreateCollectionCleanupTest extends SolrCloudTestCase { Properties properties = new Properties(); properties.put(CoreAdminParams.DATA_DIR, "/some_invalid_dir/foo"); create.setProperties(properties); - CollectionAdminResponse rsp = create.process(cloudClient); - assertFalse(rsp.isSuccess()); + expectThrows(HttpSolrClient.RemoteSolrException.class, () -> { + CollectionAdminResponse rsp = create.process(cloudClient); + }); // Confirm using LIST that the collection does not exist assertThat("Failed collection is still in the clusterstate: " + cluster.getSolrClient().getClusterStateProvider().getClusterState().getCollectionOrNull(collectionName), diff --git a/solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java b/solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java index f513645b05f..5933f8f7f7f 100644 --- a/solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java +++ b/solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java @@ -61,6 +61,7 @@ import org.apache.solr.client.solrj.SolrRequest; import org.apache.solr.client.solrj.SolrServerException; import org.apache.solr.client.solrj.embedded.JettySolrRunner; import org.apache.solr.client.solrj.impl.CloudSolrClient; +import org.apache.solr.client.solrj.impl.HttpSolrClient; import org.apache.solr.client.solrj.impl.HttpClientUtil; import org.apache.solr.client.solrj.request.ConfigSetAdminRequest; import org.apache.solr.client.solrj.request.ConfigSetAdminRequest.Create; @@ -89,6 +90,7 @@ import org.apache.zookeeper.KeeperException; import org.junit.After; import org.junit.Before; import org.junit.Test; +import static org.junit.matchers.JUnitMatchers.containsString; import org.noggit.JSONParser; import org.noggit.ObjectBuilder; import org.slf4j.Logger; @@ -344,29 +346,26 @@ public class TestConfigSetsAPI extends SolrTestCaseJ4 { @Test public void testUploadWithScriptUpdateProcessor() throws Exception { - for (boolean withAuthorization: Arrays.asList(false, true)) { - String suffix; - if (withAuthorization) { - suffix = "-trusted"; - protectConfigsHandler(); - uploadConfigSetWithAssertions("with-script-processor", suffix, "solr", "SolrRocks"); - } else { - suffix = "-untrusted"; - uploadConfigSetWithAssertions("with-script-processor", suffix, null, null); - } + // Authorization off + final String untrustedSuffix = "-untrusted"; + uploadConfigSetWithAssertions("with-script-processor", untrustedSuffix, null, null); // try to create a collection with the uploaded configset - CollectionAdminResponse resp = createCollection("newcollection2", "with-script-processor"+suffix, - 1, 1, solrCluster.getSolrClient()); - - if (withAuthorization) { - scriptRequest("newcollection2"); - } else { - log.info("Client saw errors: "+resp.getErrorMessages()); - assertTrue(resp.getErrorMessages() != null && resp.getErrorMessages().size() > 0); - assertTrue(resp.getErrorMessages().getVal(0). - contains("The configset for this collection was uploaded without any authentication")); - } - } + Throwable thrown = expectThrows(HttpSolrClient.RemoteSolrException.class, () -> { + createCollection("newcollection2", "with-script-processor" + untrustedSuffix, + 1, 1, solrCluster.getSolrClient()); + }); + + assertThat(thrown.getMessage(), containsString("Underlying core creation failed")); + + // Authorization on + final String trustedSuffix = "-trusted"; + protectConfigsHandler(); + uploadConfigSetWithAssertions("with-script-processor", trustedSuffix, "solr", "SolrRocks"); + // try to create a collection with the uploaded configset + CollectionAdminResponse resp = createCollection("newcollection2", "with-script-processor" + trustedSuffix, + 1, 1, solrCluster.getSolrClient()); + scriptRequest("newcollection2"); + } protected SolrZkClient zkClient() { diff --git a/solr/core/src/test/org/apache/solr/cloud/api/collections/CollectionsAPIDistributedZkTest.java b/solr/core/src/test/org/apache/solr/cloud/api/collections/CollectionsAPIDistributedZkTest.java index 4718a109ac7..5c3d73c6a1f 100644 --- a/solr/core/src/test/org/apache/solr/cloud/api/collections/CollectionsAPIDistributedZkTest.java +++ b/solr/core/src/test/org/apache/solr/cloud/api/collections/CollectionsAPIDistributedZkTest.java @@ -286,19 +286,11 @@ public class CollectionsAPIDistributedZkTest extends SolrCloudTestCase { String nn1 = cluster.getJettySolrRunner(0).getNodeName(); String nn2 = cluster.getJettySolrRunner(1).getNodeName(); - CollectionAdminResponse resp = CollectionAdminRequest.createCollection("halfcollection", "conf", 2, 1) - .setCreateNodeSet(nn1 + "," + nn2) - .process(cluster.getSolrClient()); - - SimpleOrderedMap success = (SimpleOrderedMap) resp.getResponse().get("success"); - SimpleOrderedMap failure = (SimpleOrderedMap) resp.getResponse().get("failure"); - - assertNotNull(resp.toString(), success); - assertNotNull(resp.toString(), failure); - - String val1 = success.getVal(0).toString(); - String val2 = failure.getVal(0).toString(); - assertTrue(val1.contains("SolrException") || val2.contains("SolrException")); + expectThrows(HttpSolrClient.RemoteSolrException.class, () -> { + CollectionAdminResponse resp = CollectionAdminRequest.createCollection("halfcollection", "conf", 2, 1) + .setCreateNodeSet(nn1 + "," + nn2) + .process(cluster.getSolrClient()); + }); } @Test 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 531e154fd2a..2adc13e91ed 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 @@ -25,13 +25,16 @@ import java.util.Map; import java.util.concurrent.atomic.AtomicLong; import com.google.common.collect.Lists; +import org.apache.solr.cloud.ZkTestServer; import org.apache.solr.client.solrj.SolrQuery; import org.apache.solr.client.solrj.SolrRequest; import org.apache.solr.client.solrj.SolrServerException; import org.apache.solr.client.solrj.impl.CloudSolrClient; +import org.apache.solr.client.solrj.impl.HttpSolrClient; import org.apache.solr.client.solrj.impl.HttpSolrClient.RemoteSolrException; import org.apache.solr.client.solrj.request.CollectionAdminRequest; import org.apache.solr.client.solrj.request.QueryRequest; +import org.apache.solr.client.solrj.response.CollectionAdminResponse; import org.apache.solr.client.solrj.request.V2Request; import org.apache.solr.client.solrj.response.QueryResponse; import org.apache.solr.common.SolrException; @@ -40,6 +43,7 @@ import org.apache.solr.common.cloud.ClusterState; import org.apache.solr.common.cloud.DocCollection; import org.apache.solr.common.cloud.Replica; import org.apache.solr.common.cloud.Slice; +import org.apache.solr.common.cloud.SolrZkClient; import org.apache.solr.common.params.CollectionParams; import org.apache.solr.common.params.ModifiableSolrParams; import org.apache.solr.common.params.ShardParams; @@ -971,4 +975,34 @@ public class TestCollectionAPI extends ReplicaPropertiesBase { se.getMessage().toLowerCase(Locale.ROOT).contains("missing required parameter:")); } } + + /** + * After a failed attempt to create a collection (due to bad configs), assert that + * the collection can be created with a good collection. + */ + @Test + @ShardsFixed(num = 2) + public void testRecreateCollectionAfterFailure() throws Exception { + // Upload a bad configset + SolrZkClient zkClient = new SolrZkClient(zkServer.getZkHost(), ZkTestServer.TIMEOUT, + ZkTestServer.TIMEOUT, null); + ZkTestServer.putConfig("badconf", zkClient, "/solr", ZkTestServer.SOLRHOME, "bad-error-solrconfig.xml", "solrconfig.xml"); + ZkTestServer.putConfig("badconf", zkClient, "/solr", ZkTestServer.SOLRHOME, "schema-minimal.xml", "schema.xml"); + zkClient.close(); + + try (CloudSolrClient client = createCloudClient(null)) { + // first, try creating a collection with badconf + HttpSolrClient.RemoteSolrException rse = expectThrows(HttpSolrClient.RemoteSolrException.class, () -> { + CollectionAdminResponse rsp = CollectionAdminRequest.createCollection + ("testcollection", "badconf", 1, 2).process(client); + }); + assertNotNull(rse.getMessage()); + assertNotSame(0, rse.code()); + + CollectionAdminResponse rsp = CollectionAdminRequest.createCollection + ("testcollection", "conf1", 1, 2).process(client); + assertNull(rsp.getErrorMessages()); + assertSame(0, rsp.getStatus()); + } + } } diff --git a/solr/test-framework/src/java/org/apache/solr/cloud/ZkTestServer.java b/solr/test-framework/src/java/org/apache/solr/cloud/ZkTestServer.java index 27c551e9525..4e5f8697901 100644 --- a/solr/test-framework/src/java/org/apache/solr/cloud/ZkTestServer.java +++ b/solr/test-framework/src/java/org/apache/solr/cloud/ZkTestServer.java @@ -788,11 +788,17 @@ public class ZkTestServer { public static void putConfig(String confName, SolrZkClient zkClient, File solrhome, final String name) throws Exception { - putConfig(confName, zkClient, solrhome, name, name); + putConfig(confName, zkClient, null, solrhome, name, name); } - public static void putConfig(String confName, SolrZkClient zkClient, File solrhome, final String srcName, String destName) - throws Exception { + + public static void putConfig(String confName, SolrZkClient zkClient, File solrhome, final String srcName, + String destName) throws Exception { + putConfig(confName, zkClient, null, solrhome, srcName, destName); + } + + public static void putConfig(String confName, SolrZkClient zkClient, String zkChroot, File solrhome, + final String srcName, String destName) throws Exception { File file = new File(solrhome, "collection1" + File.separator + "conf" + File.separator + srcName); if (!file.exists()) { @@ -801,6 +807,9 @@ public class ZkTestServer { } String destPath = "/configs/" + confName + "/" + destName; + if (zkChroot != null) { + destPath = zkChroot + destPath; + } log.info("put " + file.getAbsolutePath() + " to " + destPath); zkClient.makePath(destPath, file, false, true); }