SOLR-5970: Return correct status upon collection creation failure

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
 ----------------------
 

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<Object>());
         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);
 

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), 

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() {

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

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());
+    }
+  }
 }

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);
   }