dsmiley commented on a change in pull request #322:
URL: https://github.com/apache/solr/pull/322#discussion_r723654321
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -87,20 +80,61 @@ private static ConfigSetService instantiate(CoreContainer
coreContainer) {
}
}
- private static void bootstrapDefaultConfigSet(ConfigSetService
configSetService) throws IOException {
- if (configSetService.checkConfigExists("_default") == false) {
+ private void bootstrapConfigSet(CoreContainer coreContainer) {
+ // bootstrap _default conf, bootstrap_confdir and bootstrap_conf if
provided via system property
+ try {
+ // _default conf
+ bootstrapDefaultConf();
+
+ // bootstrap_confdir
+ String confDir = System.getProperty("bootstrap_confdir");
+ if (confDir != null) {
+ bootstrapConfDir(confDir);
+ }
+
+ // bootstrap_conf, in SolrCloud mode
+ boolean boostrapConf = Boolean.getBoolean("bootstrap_conf");
+ if (boostrapConf == true) {
+ if (coreContainer.getZkController() != null) {
+ bootstrapConf(coreContainer);
+ }
+ }
+ } catch (UnsupportedOperationException e) {
+ log.info("Not bootstrapping configSets because they are read-only");
+ } catch (IOException e) {
+ throw new SolrException(
+ SolrException.ErrorCode.SERVER_ERROR, "Config couldn't be uploaded
", e);
+ }
+ }
+
+ private void bootstrapDefaultConf() throws IOException {
+ if (this.checkConfigExists("_default") == false) {
String configDirPath = getDefaultConfigDirPath();
if (configDirPath == null) {
log.warn(
- "The _default configset could not be uploaded. Please provide
'solr.default.confdir' parameter that points to a configset {} {}",
- "intended to be the default. Current 'solr.default.confdir'
value:",
-
System.getProperty(SolrDispatchFilter.SOLR_DEFAULT_CONFDIR_ATTRIBUTE));
+ "The _default configset could not be uploaded. Please provide
'solr.default.confdir' parameter that points to a configset {} {}",
+ "intended to be the default. Current 'solr.default.confdir'
value:",
+
System.getProperty(SolrDispatchFilter.SOLR_DEFAULT_CONFDIR_ATTRIBUTE));
} else {
-
configSetService.uploadConfig(ConfigSetsHandler.DEFAULT_CONFIGSET_NAME,
Paths.get(configDirPath));
+ this.uploadConfig(ConfigSetsHandler.DEFAULT_CONFIGSET_NAME,
Paths.get(configDirPath));
}
}
}
+ private void bootstrapConfDir(String confDir) throws IOException {
+ Path configPath = Paths.get(confDir);
+ if (!Files.isDirectory(configPath)) {
+ throw new IllegalArgumentException(
+ String.format(
Review comment:
Why use String.format instead of simple String concatenation with `+` ?
##########
File path:
solr/solrj/src/test/org/apache/solr/common/cloud/TestZkConfigSetService.java
##########
@@ -223,24 +211,23 @@ public void testUploadWithACL() throws IOException {
}
@Test
- public void testBootstrapConf() throws IOException {
+ public void testBootstrapConf() throws IOException, KeeperException,
InterruptedException {
+
+ String solrHome = SolrJettyTestBase.legacyExampleCollection1SolrHome();
+
CoreContainer cc = new CoreContainer(Paths.get(solrHome), new
Properties());
+ System.setProperty("zkHost", zkServer.getZkAddress());
+
+ SolrZkClient zkClient = new SolrZkClient(zkServer.getZkHost(),
AbstractZkTestCase.TIMEOUT);
+ zkClient.makePath("/solr", false, true);
cc.setCoreConfigService(new ZkConfigSetService(zkClient));
ConfigSetService.bootstrapConf(cc);
assertTrue(cc.getConfigSetService().checkConfigExists("collection1"));
Review comment:
Also check that this configSet did *not* exist previously.
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -78,28 +80,30 @@ private static ConfigSetService instantiate(CoreContainer
coreContainer) {
}
}
- public void bootstrapConfigSet() {
+ private void bootstrapConfigSet(CoreContainer coreContainer) {
// 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
+ String confDir = System.getProperty("bootstrap_confdir");
if (confDir != null) {
bootstrapConfDir(confDir);
}
+
// bootstrap_conf, in SolrCloud mode
+ boolean boostrapConf = Boolean.getBoolean("bootstrap_conf");
if (boostrapConf == true) {
- if (this instanceof ZkConfigSetService) {
- bootstrapConf(((ZkConfigSetService)
this).getZkController().getCoreContainer());
+ if (coreContainer.getZkController() != null) {
+ bootstrapConf(coreContainer);
}
}
} catch (UnsupportedOperationException e) {
- log.info("config couldn't be uploaded");
+ log.info("Not bootstrapping configSets because they are read-only");
Review comment:
```suggestion
log.info("Not bootstrapping configSets because the ConfigSetService is
apparently read-only");
```
Even then I have questions. It seems very hypothetical to get this
exception, so should we bother? I'd rather skip it. Secondly, lets say
someone is using FileSystemConfigSetService and the target dir is read-only.
What would happen? Probably some sort of IOException. I suppose that's fair
(not something to ignore) since the user is requesting something that would
lead to an upload and thus they want/intend for it. Well, `_default` is
implicit but assuming this config is already there, then we wouldn't get an
error since we check for it, I see.
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -78,28 +80,30 @@ private static ConfigSetService instantiate(CoreContainer
coreContainer) {
}
}
- public void bootstrapConfigSet() {
+ private void bootstrapConfigSet(CoreContainer coreContainer) {
// 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
+ String confDir = System.getProperty("bootstrap_confdir");
if (confDir != null) {
bootstrapConfDir(confDir);
}
+
// bootstrap_conf, in SolrCloud mode
+ boolean boostrapConf = Boolean.getBoolean("bootstrap_conf");
if (boostrapConf == true) {
- if (this instanceof ZkConfigSetService) {
- bootstrapConf(((ZkConfigSetService)
this).getZkController().getCoreContainer());
+ if (coreContainer.getZkController() != null) {
Review comment:
Instead of merely doing this just for bootstrap_conf, shouldn't we do it
for everything else? Thus have the caller (createConfigSetService) not even
call bootstrapConfigSet unless we're in SolrCloud.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
