dsmiley commented on a change in pull request #322:
URL: https://github.com/apache/solr/pull/322#discussion_r723257144
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -751,6 +751,7 @@ public void load() {
warnUsersOfInsecureSettings();
this.backupRepoFactory = new
BackupRepositoryFactory(cfg.getBackupRepositoryPlugins());
coreConfigService = ConfigSetService.createConfigSetService(this);
+ coreConfigService.bootstrapConfigSet();
Review comment:
why do this here and not in ConfigSetService.createConfigSetService?
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -87,20 +78,59 @@ private static ConfigSetService instantiate(CoreContainer
coreContainer) {
}
}
- private static void bootstrapDefaultConfigSet(ConfigSetService
configSetService) throws IOException {
- if (configSetService.checkConfigExists("_default") == false) {
+ public void bootstrapConfigSet() {
+ // bootstrap _default conf, bootstrap_confdir and bootstrap_conf if
provided via system property
+ String confDir = System.getProperty("bootstrap_confdir");
+ boolean boostrapConf = Boolean.getBoolean("bootstrap_conf");
+ try {
+ // _default conf
+ bootstrapDefaultConf();
+ // bootstrap_confdir
+ if (confDir != null) {
+ bootstrapConfDir(confDir);
+ }
+ // bootstrap_conf, in SolrCloud mode
+ if (boostrapConf == true) {
+ if (this instanceof ZkConfigSetService) {
Review comment:
yuck; why not have bootstrapConfigSet take a CoreContainer arg?
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -87,20 +78,59 @@ private static ConfigSetService instantiate(CoreContainer
coreContainer) {
}
}
- private static void bootstrapDefaultConfigSet(ConfigSetService
configSetService) throws IOException {
- if (configSetService.checkConfigExists("_default") == false) {
+ public void bootstrapConfigSet() {
+ // bootstrap _default conf, bootstrap_confdir and bootstrap_conf if
provided via system property
+ String confDir = System.getProperty("bootstrap_confdir");
Review comment:
It's better to declare variables as late as possible (just-in-time when
needed), not at early as possible
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -87,20 +78,59 @@ private static ConfigSetService instantiate(CoreContainer
coreContainer) {
}
}
- private static void bootstrapDefaultConfigSet(ConfigSetService
configSetService) throws IOException {
- if (configSetService.checkConfigExists("_default") == false) {
+ public void bootstrapConfigSet() {
+ // bootstrap _default conf, bootstrap_confdir and bootstrap_conf if
provided via system property
+ String confDir = System.getProperty("bootstrap_confdir");
+ boolean boostrapConf = Boolean.getBoolean("bootstrap_conf");
+ try {
+ // _default conf
+ bootstrapDefaultConf();
+ // bootstrap_confdir
+ if (confDir != null) {
+ bootstrapConfDir(confDir);
+ }
+ // bootstrap_conf, in SolrCloud mode
Review comment:
good point that bootstrap_conf could only make sense in SolrCloud. So
check that we're in SolrCloud mode. The typical way to do this is check for
the existence coreContainer.getZkController
Come to think of it, all of bootstrapConfigSet should be guarded by this.
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -87,20 +78,59 @@ private static ConfigSetService instantiate(CoreContainer
coreContainer) {
}
}
- private static void bootstrapDefaultConfigSet(ConfigSetService
configSetService) throws IOException {
- if (configSetService.checkConfigExists("_default") == false) {
+ public void bootstrapConfigSet() {
+ // bootstrap _default conf, bootstrap_confdir and bootstrap_conf if
provided via system property
+ String confDir = System.getProperty("bootstrap_confdir");
+ boolean boostrapConf = Boolean.getBoolean("bootstrap_conf");
+ try {
+ // _default conf
+ bootstrapDefaultConf();
+ // bootstrap_confdir
+ if (confDir != null) {
+ bootstrapConfDir(confDir);
+ }
+ // bootstrap_conf, in SolrCloud mode
+ if (boostrapConf == true) {
+ if (this instanceof ZkConfigSetService) {
+ bootstrapConf(((ZkConfigSetService)
this).getZkController().getCoreContainer());
+ }
+ }
+ } catch (UnsupportedOperationException e) {
+ log.info("config couldn't be uploaded");
Review comment:
```suggestion
log.info("Not bootstrapping configSets because they are read-only");
```
##########
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:
As we discussed in person, I'm saying the setup/teardown logic should be
_inlined_ into where you are moving the test, basically.
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
