murblanc commented on code in PR #1891:
URL: https://github.com/apache/solr/pull/1891#discussion_r1314992428
##########
solr/core/src/java/org/apache/solr/core/CoresLocator.java:
##########
@@ -72,4 +73,26 @@ public interface CoresLocator {
* @return a list of all CoreDescriptors found
*/
public List<CoreDescriptor> 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<? extends CoresLocator> clazz =
+ nodeConfig.getSolrResourceLoader().findClass(coresLocatorClass,
CoresLocator.class);
+ Constructor<? extends CoresLocator> 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());
Review Comment:
Maybe add a constructor taking single param `NodeConfig nodeConfig` to
`CorePropertiesLocator` so that it can be configured and instantiated like
other `CoresLocator` implementations?
##########
solr/core/src/java/org/apache/solr/core/NodeConfig.java:
##########
@@ -154,6 +155,7 @@ private NodeConfig(
Set<Path> allowPaths,
List<String> allowUrls,
String configSetServiceClass,
+ String coresLocatorClass,
Review Comment:
Minor/personal taste: I'd add this closer to existing `coreRootDirectory`
given the two relate to `CoreLocator` implementation configuration.
And I wish(ed) the order of attribute processing in `SolrXmlConfig` was the
same as the order here, but it doesn't seem to be the case.
##########
solr/solr-ref-guide/modules/configuration-guide/pages/configuring-solr-xml.adoc:
##########
@@ -99,6 +99,18 @@ For example, `<str
name="configSetService">com.myorg.CustomConfigSetService</str
+
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`.
Review Comment:
I'd say "implements" rather than "inherits from" given `CoresLocator` is an
interface.
Also wording "and you must provide a constructor with one parameter which
the type is `org.apache.solr.core.NodeConfig`" --> " and that provides a
constructor with one parameter of type `org.apache.solr.core.NodeConfig`"
--
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
