[GitHub] [solr] dsmiley commented on a change in pull request #322: SOLR-15258: ConfigSetService supports bootstrap_conf

[GitBox <gi...@apache.org>]

dsmiley commented on a change in pull request #322:
URL: https://github.com/apache/solr/pull/322#discussion_r719579038



##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -54,6 +54,28 @@
 
   public static ConfigSetService createConfigSetService(CoreContainer coreContainer) {
     final ConfigSetService configSetService = instantiate(coreContainer);
+
+    // bootstrap conf if provided

Review comment:
       create is now bootstrapping three things: bootstrap_conf, bootstrap_confdir, and the `_default`.  Let's bemore consistent -- either have one method doing these, or one method for each.  In this PR it's lopsided and a try-catch around two of those which feels arbitrary.

##########
File path: solr/solrj/src/test/org/apache/solr/common/cloud/TestZkConfigSetService.java
##########
@@ -16,45 +16,63 @@
  */
 package org.apache.solr.common.cloud;
 
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.security.NoSuchAlgorithmException;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Properties;
+
 import com.google.common.base.Throwables;
+import org.apache.solr.SolrJettyTestBase;
 import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.cloud.AbstractZkTestCase;
 import org.apache.solr.cloud.ZkConfigSetService;
 import org.apache.solr.cloud.ZkTestServer;
 import org.apache.solr.core.ConfigSetService;
+import org.apache.solr.core.CoreContainer;
 import org.apache.zookeeper.KeeperException;
 import org.apache.zookeeper.ZooDefs;
 import org.apache.zookeeper.data.ACL;
 import org.apache.zookeeper.data.Id;
 import org.apache.zookeeper.server.auth.DigestAuthenticationProvider;
-import org.junit.AfterClass;
-import org.junit.BeforeClass;
 import org.junit.Test;
 
-import java.io.IOException;
-import java.nio.charset.StandardCharsets;
-import java.nio.file.Files;
-import java.nio.file.Path;
-import java.security.NoSuchAlgorithmException;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.List;
-
 public class TestZkConfigSetService extends SolrTestCaseJ4 {
 
-  private static ZkTestServer zkServer;
+  private ZkTestServer zkServer;
+  protected Path zkDir;
+
+  private String solrHome;
 
-  @BeforeClass
-  public static void startZkServer() throws Exception {
-    zkServer = new ZkTestServer(createTempDir("zkData"));
+  private SolrZkClient zkClient;
+
+
+  @Override
+  public void setUp() throws Exception {

Review comment:
       The added setup & teardown is only used by one test, thus doing wasted stuff for the other tests.  You can move it to testBootstrapConf

##########
File path: solr/solrj/src/test/org/apache/solr/common/cloud/TestZkConfigSetService.java
##########
@@ -205,6 +223,25 @@ public void testUploadWithACL() throws IOException {
 
   }
 
+  @Test
+  public void testBootstrapConf() throws IOException {
+    CoreContainer cc = new CoreContainer(Paths.get(solrHome), new Properties());
+    cc.setCoreConfigService(new ZkConfigSetService(zkClient));
+    ConfigSetService.bootstrapConf(cc);
+    assertEquals(cc.getConfigSetService().listConfigs().size(), 1);

Review comment:
       assert the specific collection we expect to see.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org