SOLR-16959: Make CoresLocator class configurable (#1891)

Co-authored-by: Vincent Primault <[email protected]>
(cherry picked from commit 708ab9b)

solr/CHANGES.txt

@@ -82,6 +82,8 @@ Improvements
 
 * SOLR-15440: The Learning To Rank FieldValueFeature now uses DocValues when docValues=true and stored=true are combined. (Christine Poerschke, Tom Gilke)
 
+* SOLR-16959: Make the internal CoresLocator implementation configurable in solr.xml (Vincent Primault via David Smiley)
+
 Optimizations
 ---------------------
 

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;
@@ -379,21 +378,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.
+   *
+   * <p>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);
   }
@@ -1949,9 +1951,7 @@ private CoreDescriptor reloadCoreDescriptor(CoreDescriptor oldDesc) {
       return null;
     }
 
-    CorePropertiesLocator cpl = new CorePropertiesLocator(null);
-    CoreDescriptor ret =
-        cpl.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

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,6 +199,11 @@ public FileVisitResult visitFileFailed(Path file, IOException exc) throws IOExce
     return cds;
   }
 
+  @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

solr/core/src/java/org/apache/solr/core/CoresLocator.java

@@ -27,7 +27,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
@@ -36,7 +36,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
@@ -45,7 +45,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
@@ -54,7 +54,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
@@ -63,13 +63,39 @@ 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
    *
    * @param cc the CoreContainer
    * @return a list of all CoreDescriptors found
    */
-  public List<CoreDescriptor> discover(CoreContainer cc);
+  List<CoreDescriptor> 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.
+   *
+   * @param nodeConfig Solr configuration.
+   */
+  static CoresLocator instantiate(NodeConfig nodeConfig) {
+    final String coresLocatorClass = nodeConfig.getCoresLocatorClass();
+    return nodeConfig
+        .getSolrResourceLoader()
+        .newInstance(
+            coresLocatorClass,
+            CoresLocator.class,
+            null,
+            new Class<?>[] {NodeConfig.class},
+            new Object[] {nodeConfig});
+  }
 }

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;
 
@@ -125,6 +126,7 @@ public class NodeConfig {
   private NodeConfig(
       String nodeName,
       Path coreRootDirectory,
+      String coresLocatorClass,
       Path solrDataHome,
       Integer booleanQueryMaxClauseCount,
       Path configSetBaseDirectory,
@@ -162,6 +164,7 @@ private NodeConfig(
     // 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;
@@ -271,6 +274,10 @@ public Path getCoreRootDirectory() {
     return coreRootDirectory;
   }
 
+  public String getCoresLocatorClass() {
+    return this.coresLocatorClass;
+  }
+
   /** Absolute. */
   public Path getSolrDataHome() {
     return solrDataHome;
@@ -592,6 +599,7 @@ public static class NodeConfigBuilder {
     // all Path fields here are absolute and normalized.
     private SolrResourceLoader loader;
     private Path coreRootDirectory;
+    private String coresLocatorClass = DEFAULT_CORESLOCATORCLASS;
     private Path solrDataHome;
     private Integer booleanQueryMaxClauseCount;
     private Path configSetBaseDirectory;
@@ -632,6 +640,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 =
@@ -671,6 +681,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()) {
@@ -755,8 +770,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;
@@ -886,6 +901,7 @@ public NodeConfig build() {
       return new NodeConfig(
           nodeName,
           coreRootDirectory,
+          coresLocatorClass,
           solrDataHome,
           booleanQueryMaxClauseCount,
           configSetBaseDirectory,

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;

solr/core/src/test/org/apache/solr/core/TestCoreContainer.java

@@ -770,6 +770,45 @@ public void testCustomConfigSetService() throws Exception {
     }
   }
 
+  @Test
+  public void testDefaultCoresLocator() throws Exception {
+    String solrXml = "<?xml version=\"1.0\" encoding=\"UTF-8\" ?><solr></solr>";
+    CoreContainer cc = init(solrXml);
+    try {
+      assertTrue(cc.getCoresLocator() instanceof CorePropertiesLocator);
+    } finally {
+      cc.shutdown();
+    }
+  }
+
+  @Test
+  public void testCustomCoresLocator() throws Exception {
+    String solrXml =
+        "<?xml version=\"1.0\" encoding=\"UTF-8\" ?>\n"
+            + "<solr>\n"
+            + "<str name=\"coresLocator\">org.apache.solr.core.TestCoreContainer$CustomCoresLocator</str>\n"
+            + "</solr>";
+    CoreContainer cc = init(solrXml);
+    try {
+      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<CoreDescriptor> cores = new ArrayList<>();
@@ -799,6 +838,11 @@ public void swap(CoreContainer cc, CoreDescriptor cd1, CoreDescriptor cd2) {}
     public List<CoreDescriptor> discover(CoreContainer cc) {
       return cores;
     }
+
+    @Override
+    public CoreDescriptor reload(CoreDescriptor cd, CoreContainer cc) {
+      return cd;
+    }
   }
 
   @Test

solr/core/src/test/org/apache/solr/core/TestLazyCores.java

@@ -743,7 +743,7 @@ private CoreContainer initGoodAndBad(
     NodeConfig config = SolrXmlConfig.fromString(solrHomeDirectory.toPath(), "<solr/>");
 
     // 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

solr/solr-ref-guide/modules/configuration-guide/pages/configuring-solr-xml.adoc

@@ -95,7 +95,7 @@ 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, `<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`.
@@ -189,6 +189,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.CorePropertiesLocator`
+|===
++
+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, `<str name="coresLocator">com.myorg.CustomCoresLocator</str>`.
+
 `managementPath`::
 +
 [%autowidth,frame=none]

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;
+  }
 }

solr/test-framework/src/java/org/apache/solr/util/TestHarness.java

@@ -175,7 +175,7 @@ public TestHarness(Path solrHome, String solrXml) {
   }
 
   public TestHarness(NodeConfig nodeConfig) {
-    this(nodeConfig, new CorePropertiesLocator(nodeConfig.getCoreRootDirectory()));
+    this(nodeConfig, new CorePropertiesLocator(nodeConfig));
   }
 
   /**