From fe875669cb7b64e16a54c9d3f9642ae5ed6f70a5 Mon Sep 17 00:00:00 2001 From: Vincent Primault Date: Mon, 4 Sep 2023 15:21:16 +0200 Subject: [PATCH 1/8] SOLR-16959: Make CoresContainer class configurable --- .../org/apache/solr/core/CoreContainer.java | 11 ++-- .../org/apache/solr/core/CoresLocator.java | 23 ++++++++ .../java/org/apache/solr/core/NodeConfig.java | 14 +++++ .../org/apache/solr/core/SolrXmlConfig.java | 3 ++ .../apache/solr/core/CoresLocatorTest.java | 54 +++++++++++++++++++ .../pages/configuring-solr-xml.adoc | 12 +++++ .../org/apache/solr/util/NoCoresLocator.java | 40 ++++++++++++++ 7 files changed, 153 insertions(+), 4 deletions(-) create mode 100644 solr/core/src/test/org/apache/solr/core/CoresLocatorTest.java create mode 100644 solr/test-framework/src/java/org/apache/solr/util/NoCoresLocator.java diff --git a/solr/core/src/java/org/apache/solr/core/CoreContainer.java b/solr/core/src/java/org/apache/solr/core/CoreContainer.java index 0bc9ad59d27..ba66dae7b4d 100644 --- a/solr/core/src/java/org/apache/solr/core/CoreContainer.java +++ b/solr/core/src/java/org/apache/solr/core/CoreContainer.java @@ -377,21 +377,24 @@ public CoreContainer(Path solrHome, Properties properties) { * @see #load() */ public CoreContainer(NodeConfig config) { - this(config, new CorePropertiesLocator(config.getCoreRootDirectory())); + this(config, CoresLocator.instantiate(config)); } public CoreContainer(NodeConfig config, boolean asyncSolrCoreLoad) { - this(config, new CorePropertiesLocator(config.getCoreRootDirectory()), asyncSolrCoreLoad); + this(config, CoresLocator.instantiate(config), asyncSolrCoreLoad); } /** - * Create a new CoreContainer using the given configuration and locator. The container's cores are - * not loaded. + * Create a new CoreContainer using the given configuration and locator. + * + *

The container's cores are not loaded. This constructor should be used only in tests, as it + * overrides {@link CoresLocator}'s instantiation process. * * @param config a ConfigSolr representation of this container's configuration * @param locator a CoresLocator * @see #load() */ + @VisibleForTesting public CoreContainer(NodeConfig config, CoresLocator locator) { this(config, locator, false); } diff --git a/solr/core/src/java/org/apache/solr/core/CoresLocator.java b/solr/core/src/java/org/apache/solr/core/CoresLocator.java index 10eac005230..c99c348fada 100644 --- a/solr/core/src/java/org/apache/solr/core/CoresLocator.java +++ b/solr/core/src/java/org/apache/solr/core/CoresLocator.java @@ -16,6 +16,7 @@ */ package org.apache.solr.core; +import java.lang.reflect.Constructor; import java.util.List; /** Manage the discovery and persistence of core definitions across Solr restarts */ @@ -72,4 +73,26 @@ public interface CoresLocator { * @return a list of all CoreDescriptors found */ public List discover(CoreContainer cc); + + /** + * Returns a new instance of {@link CoresLocator}, depending on provided config. + * + * @param nodeConfig Solr configuration. + */ + static CoresLocator instantiate(NodeConfig nodeConfig) { + final String coresLocatorClass = nodeConfig.getCoresLocatorClass(); + if (coresLocatorClass != null) { + try { + Class clazz = + nodeConfig.getSolrResourceLoader().findClass(coresLocatorClass, CoresLocator.class); + Constructor constructor = clazz.getConstructor(NodeConfig.class); + return constructor.newInstance(nodeConfig); + } catch (Exception e) { + throw new RuntimeException( + "create CoresLocator instance failed, coresLocatorClass=" + coresLocatorClass, e); + } + } else { + return new CorePropertiesLocator(nodeConfig.getCoreRootDirectory()); + } + } } diff --git a/solr/core/src/java/org/apache/solr/core/NodeConfig.java b/solr/core/src/java/org/apache/solr/core/NodeConfig.java index 581a2d07c0a..1fa300dff28 100644 --- a/solr/core/src/java/org/apache/solr/core/NodeConfig.java +++ b/solr/core/src/java/org/apache/solr/core/NodeConfig.java @@ -79,6 +79,7 @@ public class NodeConfig { private final UpdateShardHandlerConfig updateShardHandlerConfig; private final String configSetServiceClass; + private final String coresLocatorClass; private final String coreAdminHandlerClass; @@ -154,6 +155,7 @@ private NodeConfig( Set allowPaths, List allowUrls, String configSetServiceClass, + String coresLocatorClass, String modules, Set hiddenSysProps) { // all Path params here are absolute and normalized. @@ -190,6 +192,7 @@ private NodeConfig( this.allowPaths = allowPaths; this.allowUrls = allowUrls; this.configSetServiceClass = configSetServiceClass; + this.coresLocatorClass = coresLocatorClass; this.modules = modules; this.hiddenSysProps = hiddenSysProps; this.hiddenSysPropPattern = @@ -258,6 +261,10 @@ public String getConfigSetServiceClass() { return this.configSetServiceClass; } + public String getCoresLocatorClass() { + return this.coresLocatorClass; + } + public String getNodeName() { return nodeName; } @@ -593,6 +600,7 @@ public static class NodeConfigBuilder { private PluginInfo shardHandlerFactoryConfig; private UpdateShardHandlerConfig updateShardHandlerConfig = UpdateShardHandlerConfig.DEFAULT; private String configSetServiceClass; + private String coresLocatorClass; private String coreAdminHandlerClass = DEFAULT_ADMINHANDLERCLASS; private Map coreAdminHandlerActions = Collections.emptyMap(); private String collectionsAdminHandlerClass = DEFAULT_COLLECTIONSHANDLERCLASS; @@ -814,6 +822,11 @@ public NodeConfigBuilder setConfigSetServiceClass(String configSetServiceClass) return this; } + public NodeConfigBuilder setCoresLocatorClass(String coresLocatorClass) { + this.coresLocatorClass = coresLocatorClass; + return this; + } + /** * Set list of modules to add to class path * @@ -903,6 +916,7 @@ public NodeConfig build() { allowPaths, allowUrls, configSetServiceClass, + coresLocatorClass, modules, resolveHiddenSysPropsFromSysPropOrEnvOrDefault(hiddenSysProps)); } diff --git a/solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java b/solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java index 85fbaeeb193..c2dbca44f98 100644 --- a/solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java +++ b/solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java @@ -348,6 +348,9 @@ private static NodeConfig fillSolrSection(NodeConfig.NodeConfigBuilder builder, case "configSetService": builder.setConfigSetServiceClass(it.txt()); break; + case "coresLocator": + builder.setCoresLocatorClass(it.txt()); + break; case "coreRootDirectory": builder.setCoreRootDirectory(it.txt()); break; diff --git a/solr/core/src/test/org/apache/solr/core/CoresLocatorTest.java b/solr/core/src/test/org/apache/solr/core/CoresLocatorTest.java new file mode 100644 index 00000000000..d004883818f --- /dev/null +++ b/solr/core/src/test/org/apache/solr/core/CoresLocatorTest.java @@ -0,0 +1,54 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.core; + +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; + +import org.apache.solr.util.NoCoresLocator; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +/** Unit tests for {@link CoresLocator}. */ +public class CoresLocatorTest { + + @Rule public TemporaryFolder temporaryFolder = new TemporaryFolder(); + + @Test + public void testInstantiateDefaultCoresLocator() throws Exception { + String solrXml = ""; + NodeConfig nodeConfig = SolrXmlConfig.fromString(temporaryFolder.newFolder().toPath(), solrXml); + CoresLocator coresLocator = CoresLocator.instantiate(nodeConfig); + + assertTrue(coresLocator instanceof CorePropertiesLocator); + } + + @Test + public void testInstantiateCustomCoresLocator() throws Exception { + String solrXml = + "\n" + + "\n" + + "org.apache.solr.util.NoCoresLocator\n" + + ""; + NodeConfig nodeConfig = SolrXmlConfig.fromString(temporaryFolder.newFolder().toPath(), solrXml); + CoresLocator coresLocator = CoresLocator.instantiate(nodeConfig); + + assertTrue(coresLocator instanceof NoCoresLocator); + assertSame(nodeConfig, ((NoCoresLocator) coresLocator).getNodeConfig()); + } +} diff --git a/solr/solr-ref-guide/modules/configuration-guide/pages/configuring-solr-xml.adoc b/solr/solr-ref-guide/modules/configuration-guide/pages/configuring-solr-xml.adoc index 66fa7f8dc76..815fe6111b5 100644 --- a/solr/solr-ref-guide/modules/configuration-guide/pages/configuring-solr-xml.adoc +++ b/solr/solr-ref-guide/modules/configuration-guide/pages/configuring-solr-xml.adoc @@ -99,6 +99,18 @@ For example, `com.myorg.CustomConfigSetServicecom.myorg.CustomCoresLocator`. + `adminHandler`:: + [%autowidth,frame=none] diff --git a/solr/test-framework/src/java/org/apache/solr/util/NoCoresLocator.java b/solr/test-framework/src/java/org/apache/solr/util/NoCoresLocator.java new file mode 100644 index 00000000000..4304078233c --- /dev/null +++ b/solr/test-framework/src/java/org/apache/solr/util/NoCoresLocator.java @@ -0,0 +1,40 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.util; + +import java.util.List; +import org.apache.solr.core.CoreContainer; +import org.apache.solr.core.CoreDescriptor; +import org.apache.solr.core.NodeConfig; + +/** A {@link org.apache.solr.core.CoresLocator} that locates no cores. */ +public class NoCoresLocator extends ReadOnlyCoresLocator { + private final NodeConfig nodeConfig; + + public NoCoresLocator(NodeConfig nodeConfig) { + this.nodeConfig = nodeConfig; + } + + public NodeConfig getNodeConfig() { + return nodeConfig; + } + + @Override + public List discover(CoreContainer cc) { + return List.of(); + } +} From 7161dc6574380c0b511cf49bf08cd4db73abb0f3 Mon Sep 17 00:00:00 2001 From: Vincent Primault Date: Mon, 4 Sep 2023 16:03:20 +0200 Subject: [PATCH 2/8] test instantiation error --- .../org/apache/solr/core/CoresLocatorTest.java | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/solr/core/src/test/org/apache/solr/core/CoresLocatorTest.java b/solr/core/src/test/org/apache/solr/core/CoresLocatorTest.java index d004883818f..70eaf575e62 100644 --- a/solr/core/src/test/org/apache/solr/core/CoresLocatorTest.java +++ b/solr/core/src/test/org/apache/solr/core/CoresLocatorTest.java @@ -16,14 +16,13 @@ */ package org.apache.solr.core; -import static org.junit.Assert.assertSame; -import static org.junit.Assert.assertTrue; - import org.apache.solr.util.NoCoresLocator; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; +import static org.junit.Assert.*; + /** Unit tests for {@link CoresLocator}. */ public class CoresLocatorTest { @@ -51,4 +50,17 @@ public void testInstantiateCustomCoresLocator() throws Exception { assertTrue(coresLocator instanceof NoCoresLocator); assertSame(nodeConfig, ((NoCoresLocator) coresLocator).getNodeConfig()); } + + @Test + public void testInstantiateUnknownCoresLocator() throws Exception { + String solrXml = + "\n" + + "\n" + + "foo.BarCoresLocator\n" + + ""; + NodeConfig nodeConfig = SolrXmlConfig.fromString(temporaryFolder.newFolder().toPath(), solrXml); + Throwable t = assertThrows(RuntimeException.class, () -> CoresLocator.instantiate(nodeConfig)); + + assertEquals("create CoresLocator instance failed, coresLocatorClass=foo.BarCoresLocator", t.getMessage()); + } } From b52de708717e7ddf24eb024728a7efef50fd2596 Mon Sep 17 00:00:00 2001 From: Vincent Primault Date: Mon, 4 Sep 2023 17:25:57 +0200 Subject: [PATCH 3/8] fix review comments --- .../org/apache/solr/core/CoreContainer.java | 4 +-- .../solr/core/CorePropertiesLocator.java | 8 +++++- .../org/apache/solr/core/CoresLocator.java | 2 +- .../java/org/apache/solr/core/NodeConfig.java | 28 +++++++++---------- .../apache/solr/core/CoresLocatorTest.java | 11 ++++++-- .../org/apache/solr/core/TestLazyCores.java | 2 +- .../pages/configuring-solr-xml.adoc | 26 ++++++++--------- .../org/apache/solr/util/TestHarness.java | 2 +- 8 files changed, 47 insertions(+), 36 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/core/CoreContainer.java b/solr/core/src/java/org/apache/solr/core/CoreContainer.java index ba66dae7b4d..a0c41c4980d 100644 --- a/solr/core/src/java/org/apache/solr/core/CoreContainer.java +++ b/solr/core/src/java/org/apache/solr/core/CoreContainer.java @@ -1948,9 +1948,9 @@ private CoreDescriptor reloadCoreDescriptor(CoreDescriptor oldDesc) { return null; } - CorePropertiesLocator cpl = new CorePropertiesLocator(null); CoreDescriptor ret = - cpl.buildCoreDescriptor(oldDesc.getInstanceDir().resolve(PROPERTIES_FILENAME), this); + CorePropertiesLocator.buildCoreDescriptor( + oldDesc.getInstanceDir().resolve(PROPERTIES_FILENAME), this); // Ok, this little jewel is all because we still create core descriptors on the fly from lists // of properties in tests particularly. Theoretically, there should be _no_ way to create a diff --git a/solr/core/src/java/org/apache/solr/core/CorePropertiesLocator.java b/solr/core/src/java/org/apache/solr/core/CorePropertiesLocator.java index be3da5c3447..7c422f87af3 100644 --- a/solr/core/src/java/org/apache/solr/core/CorePropertiesLocator.java +++ b/solr/core/src/java/org/apache/solr/core/CorePropertiesLocator.java @@ -16,6 +16,7 @@ */ package org.apache.solr.core; +import com.google.common.annotations.VisibleForTesting; import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; @@ -51,6 +52,11 @@ public class CorePropertiesLocator implements CoresLocator { private final Path rootDirectory; + public CorePropertiesLocator(NodeConfig nodeConfig) { + this(nodeConfig.getCoreRootDirectory()); + } + + @VisibleForTesting public CorePropertiesLocator(Path coreDiscoveryRoot) { this.rootDirectory = coreDiscoveryRoot; log.debug("Config-defined core root directory: {}", this.rootDirectory); @@ -193,7 +199,7 @@ public FileVisitResult visitFileFailed(Path file, IOException exc) throws IOExce return cds; } - protected CoreDescriptor buildCoreDescriptor(Path propertiesFile, CoreContainer cc) { + protected static CoreDescriptor buildCoreDescriptor(Path propertiesFile, CoreContainer cc) { if (Files.notExists(propertiesFile)) { // This can happen in tests, see CoreContainer#reloadCoreDescriptor log.info("Could not load core descriptor from {} because it does not exist", propertiesFile); diff --git a/solr/core/src/java/org/apache/solr/core/CoresLocator.java b/solr/core/src/java/org/apache/solr/core/CoresLocator.java index c99c348fada..0f21b1979b9 100644 --- a/solr/core/src/java/org/apache/solr/core/CoresLocator.java +++ b/solr/core/src/java/org/apache/solr/core/CoresLocator.java @@ -92,7 +92,7 @@ static CoresLocator instantiate(NodeConfig nodeConfig) { "create CoresLocator instance failed, coresLocatorClass=" + coresLocatorClass, e); } } else { - return new CorePropertiesLocator(nodeConfig.getCoreRootDirectory()); + return new CorePropertiesLocator(nodeConfig); } } } diff --git a/solr/core/src/java/org/apache/solr/core/NodeConfig.java b/solr/core/src/java/org/apache/solr/core/NodeConfig.java index 1fa300dff28..73ecfaa2d20 100644 --- a/solr/core/src/java/org/apache/solr/core/NodeConfig.java +++ b/solr/core/src/java/org/apache/solr/core/NodeConfig.java @@ -124,6 +124,7 @@ public class NodeConfig { private NodeConfig( String nodeName, Path coreRootDirectory, + String coresLocatorClass, Path solrDataHome, Integer booleanQueryMaxClauseCount, Path configSetBaseDirectory, @@ -155,12 +156,12 @@ private NodeConfig( Set allowPaths, List allowUrls, String configSetServiceClass, - String coresLocatorClass, String modules, Set hiddenSysProps) { // all Path params here are absolute and normalized. this.nodeName = nodeName; this.coreRootDirectory = coreRootDirectory; + this.coresLocatorClass = coresLocatorClass; this.solrDataHome = solrDataHome; this.booleanQueryMaxClauseCount = booleanQueryMaxClauseCount; this.configSetBaseDirectory = configSetBaseDirectory; @@ -192,7 +193,6 @@ private NodeConfig( this.allowPaths = allowPaths; this.allowUrls = allowUrls; this.configSetServiceClass = configSetServiceClass; - this.coresLocatorClass = coresLocatorClass; this.modules = modules; this.hiddenSysProps = hiddenSysProps; this.hiddenSysPropPattern = @@ -261,10 +261,6 @@ public String getConfigSetServiceClass() { return this.configSetServiceClass; } - public String getCoresLocatorClass() { - return this.coresLocatorClass; - } - public String getNodeName() { return nodeName; } @@ -274,6 +270,10 @@ public Path getCoreRootDirectory() { return coreRootDirectory; } + public String getCoresLocatorClass() { + return this.coresLocatorClass; + } + /** Absolute. */ public Path getSolrDataHome() { return solrDataHome; @@ -591,6 +591,7 @@ public static class NodeConfigBuilder { // all Path fields here are absolute and normalized. private SolrResourceLoader loader; private Path coreRootDirectory; + private String coresLocatorClass; private Path solrDataHome; private Integer booleanQueryMaxClauseCount; private Path configSetBaseDirectory; @@ -600,7 +601,6 @@ public static class NodeConfigBuilder { private PluginInfo shardHandlerFactoryConfig; private UpdateShardHandlerConfig updateShardHandlerConfig = UpdateShardHandlerConfig.DEFAULT; private String configSetServiceClass; - private String coresLocatorClass; private String coreAdminHandlerClass = DEFAULT_ADMINHANDLERCLASS; private Map coreAdminHandlerActions = Collections.emptyMap(); private String collectionsAdminHandlerClass = DEFAULT_COLLECTIONSHANDLERCLASS; @@ -670,6 +670,11 @@ public NodeConfigBuilder setCoreRootDirectory(String coreRootDirectory) { return this; } + public NodeConfigBuilder setCoresLocatorClass(String coresLocatorClass) { + this.coresLocatorClass = coresLocatorClass; + return this; + } + public NodeConfigBuilder setSolrDataHome(String solrDataHomeString) { // keep it null unless explicitly set to non-empty value if (solrDataHomeString != null && !solrDataHomeString.isEmpty()) { @@ -754,8 +759,8 @@ public NodeConfigBuilder setReplayUpdatesThreads(int replayUpdatesThreads) { this.replayUpdatesThreads = replayUpdatesThreads; return this; } - // Remove in Solr 10.0 + @Deprecated public NodeConfigBuilder setTransientCacheSize(int transientCacheSize) { this.transientCacheSize = transientCacheSize; @@ -822,11 +827,6 @@ public NodeConfigBuilder setConfigSetServiceClass(String configSetServiceClass) return this; } - public NodeConfigBuilder setCoresLocatorClass(String coresLocatorClass) { - this.coresLocatorClass = coresLocatorClass; - return this; - } - /** * Set list of modules to add to class path * @@ -885,6 +885,7 @@ public NodeConfig build() { return new NodeConfig( nodeName, coreRootDirectory, + coresLocatorClass, solrDataHome, booleanQueryMaxClauseCount, configSetBaseDirectory, @@ -916,7 +917,6 @@ public NodeConfig build() { allowPaths, allowUrls, configSetServiceClass, - coresLocatorClass, modules, resolveHiddenSysPropsFromSysPropOrEnvOrDefault(hiddenSysProps)); } diff --git a/solr/core/src/test/org/apache/solr/core/CoresLocatorTest.java b/solr/core/src/test/org/apache/solr/core/CoresLocatorTest.java index 70eaf575e62..595654cae53 100644 --- a/solr/core/src/test/org/apache/solr/core/CoresLocatorTest.java +++ b/solr/core/src/test/org/apache/solr/core/CoresLocatorTest.java @@ -16,13 +16,16 @@ */ package org.apache.solr.core; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + import org.apache.solr.util.NoCoresLocator; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; -import static org.junit.Assert.*; - /** Unit tests for {@link CoresLocator}. */ public class CoresLocatorTest { @@ -61,6 +64,8 @@ public void testInstantiateUnknownCoresLocator() throws Exception { NodeConfig nodeConfig = SolrXmlConfig.fromString(temporaryFolder.newFolder().toPath(), solrXml); Throwable t = assertThrows(RuntimeException.class, () -> CoresLocator.instantiate(nodeConfig)); - assertEquals("create CoresLocator instance failed, coresLocatorClass=foo.BarCoresLocator", t.getMessage()); + assertEquals( + "create CoresLocator instance failed, coresLocatorClass=foo.BarCoresLocator", + t.getMessage()); } } diff --git a/solr/core/src/test/org/apache/solr/core/TestLazyCores.java b/solr/core/src/test/org/apache/solr/core/TestLazyCores.java index fbb2f5ecf57..30b038609cb 100644 --- a/solr/core/src/test/org/apache/solr/core/TestLazyCores.java +++ b/solr/core/src/test/org/apache/solr/core/TestLazyCores.java @@ -743,7 +743,7 @@ private CoreContainer initGoodAndBad( NodeConfig config = SolrXmlConfig.fromString(solrHomeDirectory.toPath(), ""); // OK this should succeed, but at the end we should have recorded a series of errors. - return createCoreContainer(config, new CorePropertiesLocator(config.getCoreRootDirectory())); + return createCoreContainer(config, new CorePropertiesLocator(config)); } // We want to see that the core "heals itself" if an un-corrupted file is written to the diff --git a/solr/solr-ref-guide/modules/configuration-guide/pages/configuring-solr-xml.adoc b/solr/solr-ref-guide/modules/configuration-guide/pages/configuring-solr-xml.adoc index 815fe6111b5..03a8e2545a8 100644 --- a/solr/solr-ref-guide/modules/configuration-guide/pages/configuring-solr-xml.adoc +++ b/solr/solr-ref-guide/modules/configuration-guide/pages/configuring-solr-xml.adoc @@ -94,23 +94,11 @@ The tables below list the child nodes of each XML element in `solr.xml`. + This attribute does not need to be set. + -If used, this attribute should be set to the FQN (Fully qualified name) of a class that inherits from `ConfigSetService`, and you must provide a constructor with one parameter which the type is `org.apache.solr.core.CoreContainer`. +If used, this attribute should be set to the FQN (Fully qualified name) of a class that inherits from `ConfigSetService`, and you must provide a constructor with one parameter of type `org.apache.solr.core.CoreContainer`. For example, `com.myorg.CustomConfigSetService`. + If this attribute isn't set, Solr uses the default `configSetService`, with zookeeper aware of `org.apache.solr.cloud.ZkConfigSetService`, without zookeeper aware of `org.apache.solr.core.FileSystemConfigSetService`. -`coresLocator`:: -+ -[%autowidth,frame=none] -|=== -|Optional |Default: `org.apache.solr.core.CoresPropertiesLocator` -|=== -+ -This attribute does not need to be set. -+ -If used, this attribute should be set to the FQN (Fully qualified name) of a class that inherits from `CoresLocator`, and you must provide a constructor with one parameter which the type is `org.apache.solr.core.NodeConfig`. -For example, `com.myorg.CustomCoresLocator`. - `adminHandler`:: + [%autowidth,frame=none] @@ -200,6 +188,18 @@ The default value is equal to the number of processors. + The root of the core discovery tree, defaults to `$SOLR_HOME`. +`coresLocator`:: ++ +[%autowidth,frame=none] +|=== +|Optional |Default: `org.apache.solr.core.CoresPropertiesLocator` +|=== ++ +This attribute does not need to be set. ++ +If used, this attribute should be set to the FQN (Fully qualified name) of a class that implements `CoresLocator`, and you must provide a constructor with one parameter of type `org.apache.solr.core.NodeConfig`. +For example, `com.myorg.CustomCoresLocator`. + `managementPath`:: + [%autowidth,frame=none] diff --git a/solr/test-framework/src/java/org/apache/solr/util/TestHarness.java b/solr/test-framework/src/java/org/apache/solr/util/TestHarness.java index 5d049e01ff4..89dd54ce5aa 100644 --- a/solr/test-framework/src/java/org/apache/solr/util/TestHarness.java +++ b/solr/test-framework/src/java/org/apache/solr/util/TestHarness.java @@ -171,7 +171,7 @@ public TestHarness(Path solrHome, String solrXml) { } public TestHarness(NodeConfig nodeConfig) { - this(nodeConfig, new CorePropertiesLocator(nodeConfig.getCoreRootDirectory())); + this(nodeConfig, new CorePropertiesLocator(nodeConfig)); } /** From b310ffe02b3b4ca506fafecfa18cb8af879f0f98 Mon Sep 17 00:00:00 2001 From: Vincent Primault Date: Mon, 4 Sep 2023 17:38:03 +0200 Subject: [PATCH 4/8] Add a reload method --- .../org/apache/solr/core/CoreContainer.java | 5 +---- .../solr/core/CorePropertiesLocator.java | 7 ++++++- .../org/apache/solr/core/CoresLocator.java | 21 +++++++++++++------ .../apache/solr/core/TestCoreContainer.java | 5 +++++ .../solr/util/ReadOnlyCoresLocator.java | 6 ++++++ 5 files changed, 33 insertions(+), 11 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/core/CoreContainer.java b/solr/core/src/java/org/apache/solr/core/CoreContainer.java index a0c41c4980d..41cd6237a15 100644 --- a/solr/core/src/java/org/apache/solr/core/CoreContainer.java +++ b/solr/core/src/java/org/apache/solr/core/CoreContainer.java @@ -26,7 +26,6 @@ import static org.apache.solr.common.params.CommonParams.METRICS_PATH; import static org.apache.solr.common.params.CommonParams.ZK_PATH; import static org.apache.solr.common.params.CommonParams.ZK_STATUS_PATH; -import static org.apache.solr.core.CorePropertiesLocator.PROPERTIES_FILENAME; import static org.apache.solr.security.AuthenticationPlugin.AUTHENTICATION_PLUGIN_PROP; import com.github.benmanes.caffeine.cache.Interner; @@ -1948,9 +1947,7 @@ private CoreDescriptor reloadCoreDescriptor(CoreDescriptor oldDesc) { return null; } - CoreDescriptor ret = - CorePropertiesLocator.buildCoreDescriptor( - oldDesc.getInstanceDir().resolve(PROPERTIES_FILENAME), this); + CoreDescriptor ret = getCoresLocator().reload(oldDesc, this); // Ok, this little jewel is all because we still create core descriptors on the fly from lists // of properties in tests particularly. Theoretically, there should be _no_ way to create a diff --git a/solr/core/src/java/org/apache/solr/core/CorePropertiesLocator.java b/solr/core/src/java/org/apache/solr/core/CorePropertiesLocator.java index 7c422f87af3..c060fc5d3cd 100644 --- a/solr/core/src/java/org/apache/solr/core/CorePropertiesLocator.java +++ b/solr/core/src/java/org/apache/solr/core/CorePropertiesLocator.java @@ -199,7 +199,12 @@ public FileVisitResult visitFileFailed(Path file, IOException exc) throws IOExce return cds; } - protected static CoreDescriptor buildCoreDescriptor(Path propertiesFile, CoreContainer cc) { + @Override + public CoreDescriptor reload(CoreDescriptor cd, CoreContainer cc) { + return buildCoreDescriptor(cd.getInstanceDir().resolve(PROPERTIES_FILENAME), cc); + } + + protected CoreDescriptor buildCoreDescriptor(Path propertiesFile, CoreContainer cc) { if (Files.notExists(propertiesFile)) { // This can happen in tests, see CoreContainer#reloadCoreDescriptor log.info("Could not load core descriptor from {} because it does not exist", propertiesFile); diff --git a/solr/core/src/java/org/apache/solr/core/CoresLocator.java b/solr/core/src/java/org/apache/solr/core/CoresLocator.java index 0f21b1979b9..79ed5dae545 100644 --- a/solr/core/src/java/org/apache/solr/core/CoresLocator.java +++ b/solr/core/src/java/org/apache/solr/core/CoresLocator.java @@ -28,7 +28,7 @@ public interface CoresLocator { * @param cc the CoreContainer * @param coreDescriptors CoreDescriptors to persist */ - public void create(CoreContainer cc, CoreDescriptor... coreDescriptors); + void create(CoreContainer cc, CoreDescriptor... coreDescriptors); /** * Ensure that the core definitions from the passed in CoreDescriptors will persist across @@ -37,7 +37,7 @@ public interface CoresLocator { * @param cc the CoreContainer * @param coreDescriptors CoreDescriptors to persist */ - public void persist(CoreContainer cc, CoreDescriptor... coreDescriptors); + void persist(CoreContainer cc, CoreDescriptor... coreDescriptors); /** * Ensure that the core definitions from the passed in CoreDescriptors are not available for @@ -46,7 +46,7 @@ public interface CoresLocator { * @param cc the CoreContainer * @param coreDescriptors CoreDescriptors of the cores to remove */ - public void delete(CoreContainer cc, CoreDescriptor... coreDescriptors); + void delete(CoreContainer cc, CoreDescriptor... coreDescriptors); /** * Persist the new name of a renamed core @@ -55,7 +55,7 @@ public interface CoresLocator { * @param oldCD the CoreDescriptor of the core before renaming * @param newCD the CoreDescriptor of the core after renaming */ - public void rename(CoreContainer cc, CoreDescriptor oldCD, CoreDescriptor newCD); + void rename(CoreContainer cc, CoreDescriptor oldCD, CoreDescriptor newCD); /** * Swap two core definitions @@ -64,7 +64,7 @@ public interface CoresLocator { * @param cd1 the core descriptor of the first core, after swapping * @param cd2 the core descriptor of the second core, after swapping */ - public void swap(CoreContainer cc, CoreDescriptor cd1, CoreDescriptor cd2); + void swap(CoreContainer cc, CoreDescriptor cd1, CoreDescriptor cd2); /** * Load all the CoreDescriptors from persistence store @@ -72,7 +72,16 @@ public interface CoresLocator { * @param cc the CoreContainer * @return a list of all CoreDescriptors found */ - public List discover(CoreContainer cc); + List discover(CoreContainer cc); + + /** + * Reload a core descriptor. + * + * @param cd the old core descriptor + * @param cc the CoreContainer + * @return a new core descriptor + */ + CoreDescriptor reload(CoreDescriptor cd, CoreContainer cc); /** * Returns a new instance of {@link CoresLocator}, depending on provided config. diff --git a/solr/core/src/test/org/apache/solr/core/TestCoreContainer.java b/solr/core/src/test/org/apache/solr/core/TestCoreContainer.java index e6b194858c9..ca30c9f1b57 100644 --- a/solr/core/src/test/org/apache/solr/core/TestCoreContainer.java +++ b/solr/core/src/test/org/apache/solr/core/TestCoreContainer.java @@ -799,6 +799,11 @@ public void swap(CoreContainer cc, CoreDescriptor cd1, CoreDescriptor cd2) {} public List discover(CoreContainer cc) { return cores; } + + @Override + public CoreDescriptor reload(CoreDescriptor cd, CoreContainer cc) { + return cd; + } } @Test diff --git a/solr/test-framework/src/java/org/apache/solr/util/ReadOnlyCoresLocator.java b/solr/test-framework/src/java/org/apache/solr/util/ReadOnlyCoresLocator.java index 29c9c41b0eb..fc04418048e 100644 --- a/solr/test-framework/src/java/org/apache/solr/util/ReadOnlyCoresLocator.java +++ b/solr/test-framework/src/java/org/apache/solr/util/ReadOnlyCoresLocator.java @@ -46,4 +46,10 @@ public void rename(CoreContainer cc, CoreDescriptor oldCD, CoreDescriptor newCD) public void swap(CoreContainer cc, CoreDescriptor cd1, CoreDescriptor cd2) { // no-op } + + @Override + public CoreDescriptor reload(CoreDescriptor cd, CoreContainer cc) { + // no-op + return cd; + } } From 83455b47cbf48528c2c67c1e30b704d74ae92953 Mon Sep 17 00:00:00 2001 From: Vincent Primault Date: Tue, 5 Sep 2023 12:42:34 +0200 Subject: [PATCH 5/8] move tests to TestCoreContainer --- .../apache/solr/core/CoresLocatorTest.java | 71 ------------------- .../apache/solr/core/TestCoreContainer.java | 28 ++++++++ 2 files changed, 28 insertions(+), 71 deletions(-) delete mode 100644 solr/core/src/test/org/apache/solr/core/CoresLocatorTest.java diff --git a/solr/core/src/test/org/apache/solr/core/CoresLocatorTest.java b/solr/core/src/test/org/apache/solr/core/CoresLocatorTest.java deleted file mode 100644 index 595654cae53..00000000000 --- a/solr/core/src/test/org/apache/solr/core/CoresLocatorTest.java +++ /dev/null @@ -1,71 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright ownership. - * The ASF licenses this file to You under the Apache License, Version 2.0 - * (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.apache.solr.core; - -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertSame; -import static org.junit.Assert.assertThrows; -import static org.junit.Assert.assertTrue; - -import org.apache.solr.util.NoCoresLocator; -import org.junit.Rule; -import org.junit.Test; -import org.junit.rules.TemporaryFolder; - -/** Unit tests for {@link CoresLocator}. */ -public class CoresLocatorTest { - - @Rule public TemporaryFolder temporaryFolder = new TemporaryFolder(); - - @Test - public void testInstantiateDefaultCoresLocator() throws Exception { - String solrXml = ""; - NodeConfig nodeConfig = SolrXmlConfig.fromString(temporaryFolder.newFolder().toPath(), solrXml); - CoresLocator coresLocator = CoresLocator.instantiate(nodeConfig); - - assertTrue(coresLocator instanceof CorePropertiesLocator); - } - - @Test - public void testInstantiateCustomCoresLocator() throws Exception { - String solrXml = - "\n" - + "\n" - + "org.apache.solr.util.NoCoresLocator\n" - + ""; - NodeConfig nodeConfig = SolrXmlConfig.fromString(temporaryFolder.newFolder().toPath(), solrXml); - CoresLocator coresLocator = CoresLocator.instantiate(nodeConfig); - - assertTrue(coresLocator instanceof NoCoresLocator); - assertSame(nodeConfig, ((NoCoresLocator) coresLocator).getNodeConfig()); - } - - @Test - public void testInstantiateUnknownCoresLocator() throws Exception { - String solrXml = - "\n" - + "\n" - + "foo.BarCoresLocator\n" - + ""; - NodeConfig nodeConfig = SolrXmlConfig.fromString(temporaryFolder.newFolder().toPath(), solrXml); - Throwable t = assertThrows(RuntimeException.class, () -> CoresLocator.instantiate(nodeConfig)); - - assertEquals( - "create CoresLocator instance failed, coresLocatorClass=foo.BarCoresLocator", - t.getMessage()); - } -} diff --git a/solr/core/src/test/org/apache/solr/core/TestCoreContainer.java b/solr/core/src/test/org/apache/solr/core/TestCoreContainer.java index ca30c9f1b57..d31f6332d91 100644 --- a/solr/core/src/test/org/apache/solr/core/TestCoreContainer.java +++ b/solr/core/src/test/org/apache/solr/core/TestCoreContainer.java @@ -47,6 +47,7 @@ import org.apache.solr.handler.admin.InfoHandler; import org.apache.solr.servlet.SolrDispatchFilter; import org.apache.solr.util.ModuleUtils; +import org.apache.solr.util.NoCoresLocator; import org.hamcrest.MatcherAssert; import org.junit.AfterClass; import org.junit.Assume; @@ -770,6 +771,33 @@ public void testCustomConfigSetService() throws Exception { } } + @Test + public void testDefaultCoresLocator() throws Exception { + String solrXml = ""; + CoreContainer cc = init(solrXml); + try { + assertTrue(cc.getCoresLocator() instanceof CorePropertiesLocator); + } finally { + cc.shutdown(); + } + } + + @Test + public void testCustomCoresLocator() throws Exception { + String solrXml = + "\n" + + "\n" + + "org.apache.solr.util.NoCoresLocator\n" + + ""; + CoreContainer cc = init(solrXml); + try { + assertTrue(cc.getCoresLocator() instanceof NoCoresLocator); + assertSame(cc.getNodeConfig(), ((NoCoresLocator) cc.getCoresLocator()).getNodeConfig()); + } finally { + cc.shutdown(); + } + } + private static class MockCoresLocator implements CoresLocator { List cores = new ArrayList<>(); From 104d1d281b4354e27187b75460c6edccbd357e18 Mon Sep 17 00:00:00 2001 From: Vincent Primault Date: Thu, 7 Sep 2023 16:16:46 +0200 Subject: [PATCH 6/8] fix way to instantiate --- .../org/apache/solr/core/CoresLocator.java | 22 +++++++------------ .../java/org/apache/solr/core/NodeConfig.java | 6 +++-- .../pages/configuring-solr-xml.adoc | 2 +- 3 files changed, 13 insertions(+), 17 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/core/CoresLocator.java b/solr/core/src/java/org/apache/solr/core/CoresLocator.java index 79ed5dae545..3321e057860 100644 --- a/solr/core/src/java/org/apache/solr/core/CoresLocator.java +++ b/solr/core/src/java/org/apache/solr/core/CoresLocator.java @@ -16,7 +16,6 @@ */ package org.apache.solr.core; -import java.lang.reflect.Constructor; import java.util.List; /** Manage the discovery and persistence of core definitions across Solr restarts */ @@ -90,18 +89,13 @@ public interface CoresLocator { */ static CoresLocator instantiate(NodeConfig nodeConfig) { final String coresLocatorClass = nodeConfig.getCoresLocatorClass(); - if (coresLocatorClass != null) { - try { - Class clazz = - nodeConfig.getSolrResourceLoader().findClass(coresLocatorClass, CoresLocator.class); - Constructor constructor = clazz.getConstructor(NodeConfig.class); - return constructor.newInstance(nodeConfig); - } catch (Exception e) { - throw new RuntimeException( - "create CoresLocator instance failed, coresLocatorClass=" + coresLocatorClass, e); - } - } else { - return new CorePropertiesLocator(nodeConfig); - } + return nodeConfig + .getSolrResourceLoader() + .newInstance( + coresLocatorClass, + CoresLocator.class, + null, + new Class[] {NodeConfig.class}, + new Object[] {nodeConfig}); } } diff --git a/solr/core/src/java/org/apache/solr/core/NodeConfig.java b/solr/core/src/java/org/apache/solr/core/NodeConfig.java index 73ecfaa2d20..39eb27e9465 100644 --- a/solr/core/src/java/org/apache/solr/core/NodeConfig.java +++ b/solr/core/src/java/org/apache/solr/core/NodeConfig.java @@ -56,6 +56,7 @@ public class NodeConfig { private final String nodeName; private final Path coreRootDirectory; + private final String coresLocatorClass; private final Path solrDataHome; @@ -79,7 +80,6 @@ public class NodeConfig { private final UpdateShardHandlerConfig updateShardHandlerConfig; private final String configSetServiceClass; - private final String coresLocatorClass; private final String coreAdminHandlerClass; @@ -591,7 +591,7 @@ public static class NodeConfigBuilder { // all Path fields here are absolute and normalized. private SolrResourceLoader loader; private Path coreRootDirectory; - private String coresLocatorClass; + private String coresLocatorClass = DEFAULT_CORESLOCATORCLASS; private Path solrDataHome; private Integer booleanQueryMaxClauseCount; private Path configSetBaseDirectory; @@ -631,6 +631,8 @@ public static class NodeConfigBuilder { // No:of core load threads in cloud mode is set to a default of 8 public static final int DEFAULT_CORE_LOAD_THREADS_IN_CLOUD = 8; + private static final String DEFAULT_CORESLOCATORCLASS = + "org.apache.solr.core.CorePropertiesLocator"; private static final String DEFAULT_ADMINHANDLERCLASS = "org.apache.solr.handler.admin.CoreAdminHandler"; private static final String DEFAULT_INFOHANDLERCLASS = diff --git a/solr/solr-ref-guide/modules/configuration-guide/pages/configuring-solr-xml.adoc b/solr/solr-ref-guide/modules/configuration-guide/pages/configuring-solr-xml.adoc index 03a8e2545a8..52fc2af843e 100644 --- a/solr/solr-ref-guide/modules/configuration-guide/pages/configuring-solr-xml.adoc +++ b/solr/solr-ref-guide/modules/configuration-guide/pages/configuring-solr-xml.adoc @@ -192,7 +192,7 @@ The root of the core discovery tree, defaults to `$SOLR_HOME`. + [%autowidth,frame=none] |=== -|Optional |Default: `org.apache.solr.core.CoresPropertiesLocator` +|Optional |Default: `org.apache.solr.core.CorePropertiesLocator` |=== + This attribute does not need to be set. From 5d1c2b35f6a3c96d7fd5132ed0b1bc77dc34c43e Mon Sep 17 00:00:00 2001 From: Vincent Primault Date: Fri, 15 Sep 2023 10:26:25 +0200 Subject: [PATCH 7/8] Simplify test --- .../apache/solr/core/TestCoreContainer.java | 19 +++++++-- .../org/apache/solr/util/NoCoresLocator.java | 40 ------------------- 2 files changed, 15 insertions(+), 44 deletions(-) delete mode 100644 solr/test-framework/src/java/org/apache/solr/util/NoCoresLocator.java diff --git a/solr/core/src/test/org/apache/solr/core/TestCoreContainer.java b/solr/core/src/test/org/apache/solr/core/TestCoreContainer.java index d31f6332d91..5e828bc9be7 100644 --- a/solr/core/src/test/org/apache/solr/core/TestCoreContainer.java +++ b/solr/core/src/test/org/apache/solr/core/TestCoreContainer.java @@ -47,7 +47,6 @@ import org.apache.solr.handler.admin.InfoHandler; import org.apache.solr.servlet.SolrDispatchFilter; import org.apache.solr.util.ModuleUtils; -import org.apache.solr.util.NoCoresLocator; import org.hamcrest.MatcherAssert; import org.junit.AfterClass; import org.junit.Assume; @@ -787,17 +786,29 @@ public void testCustomCoresLocator() throws Exception { String solrXml = "\n" + "\n" - + "org.apache.solr.util.NoCoresLocator\n" + + "org.apache.solr.core.TestCoreContainer$CustomCoresLocator\n" + ""; CoreContainer cc = init(solrXml); try { - assertTrue(cc.getCoresLocator() instanceof NoCoresLocator); - assertSame(cc.getNodeConfig(), ((NoCoresLocator) cc.getCoresLocator()).getNodeConfig()); + assertTrue(cc.getCoresLocator() instanceof CustomCoresLocator); + assertSame(cc.getNodeConfig(), ((CustomCoresLocator) cc.getCoresLocator()).getNodeConfig()); } finally { cc.shutdown(); } } + public static class CustomCoresLocator extends MockCoresLocator { + private final NodeConfig nodeConfig; + + public CustomCoresLocator(NodeConfig nodeConfig) { + this.nodeConfig = nodeConfig; + } + + public NodeConfig getNodeConfig() { + return nodeConfig; + } + } + private static class MockCoresLocator implements CoresLocator { List cores = new ArrayList<>(); diff --git a/solr/test-framework/src/java/org/apache/solr/util/NoCoresLocator.java b/solr/test-framework/src/java/org/apache/solr/util/NoCoresLocator.java deleted file mode 100644 index 4304078233c..00000000000 --- a/solr/test-framework/src/java/org/apache/solr/util/NoCoresLocator.java +++ /dev/null @@ -1,40 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright ownership. - * The ASF licenses this file to You under the Apache License, Version 2.0 - * (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.apache.solr.util; - -import java.util.List; -import org.apache.solr.core.CoreContainer; -import org.apache.solr.core.CoreDescriptor; -import org.apache.solr.core.NodeConfig; - -/** A {@link org.apache.solr.core.CoresLocator} that locates no cores. */ -public class NoCoresLocator extends ReadOnlyCoresLocator { - private final NodeConfig nodeConfig; - - public NoCoresLocator(NodeConfig nodeConfig) { - this.nodeConfig = nodeConfig; - } - - public NodeConfig getNodeConfig() { - return nodeConfig; - } - - @Override - public List discover(CoreContainer cc) { - return List.of(); - } -} From 85decb3a445ef525a679bf1deefa2549ed0b38af Mon Sep 17 00:00:00 2001 From: Vincent Date: Tue, 19 Sep 2023 08:51:45 +0200 Subject: [PATCH 8/8] Add in CHANGES.txt --- solr/CHANGES.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 13cf7644f01..19e7ec0add8 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -113,6 +113,8 @@ Improvements dedicated thread pool. Backup, Restore and Split are expensive operations. (Pierre Salagnac, David Smiley) +* SOLR-16959: Make the internal CoresLocator implementation configurable in solr.xml (Vincent Primault, David Smiley) + Optimizations ---------------------