SOLR-17269, SOLR-17386: Fixed SyntheticSolrCore reload issue (#2607)

solr/CHANGES.txt

@@ -209,7 +209,7 @@ Optimizations
 
 * SOLR-17099: Do not return spurious tags when fetching metrics from a Solr node to another. (Pierre Salagnac)
 
-* SOLR-17269: Prevent the "Coordinator node" feature from registering synthetic cores in ZooKeeper (Patson Luk)
+* SOLR-17269, SOLR-17386: Prevent the "Coordinator node" feature from registering synthetic cores in ZooKeeper (ellaeln, Patson Luk, David Smiley, Christine Poerschke)
 
 * SOLR-17330: When not set, loadOnStartup defaults to true, which is the default choice for a core. (Pierre Salagnac via Eric Pugh)
 

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

@@ -2058,9 +2058,10 @@ public void reload(String name, UUID coreId) {
 
  DocCollection docCollection = null;
  if (getZkController() != null) {
- docCollection = getZkController().getClusterState().getCollection(cd.getCollectionName());
+ docCollection =
+ getZkController().getClusterState().getCollectionOrNull(cd.getCollectionName());
  // turn off indexing now, before the new core is registered
- if (docCollection.getBool(ZkStateReader.READ_ONLY, false)) {
+ if (docCollection != null && docCollection.getBool(ZkStateReader.READ_ONLY, false)) {
  newCore.readOnly = true;
  }
  }

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

@@ -766,29 +766,16 @@ public SolrCore reload(ConfigSet coreConfig) throws IOException {
  // only one reload at a time
  synchronized (getUpdateHandler().getSolrCoreState().getReloadLock()) {
  solrCoreState.increfSolrCoreState();
- final SolrCore currentCore;
- if (!getNewIndexDir().equals(getIndexDir())) {
- // the directory is changing, don't pass on state
- currentCore = null;
- } else {
- currentCore = this;
- }
+
+ // if directory is changing, then don't pass on state
+ boolean cloneCoreState = getNewIndexDir().equals(getIndexDir());
 
  boolean success = false;
  SolrCore core = null;
  try {
  CoreDescriptor cd = new CoreDescriptor(name, getCoreDescriptor());
  cd.loadExtraProperties(); // Reload the extra properties
- core =
- new SolrCore(
- coreContainer,
- cd,
- coreConfig,
- getDataDir(),
- updateHandler,
- solrDelPolicy,
- currentCore,
- true);
+ core = cloneForReloadCore(cd, coreConfig, cloneCoreState);
 
  // we open a new IndexWriter to pick up the latest config
  core.getUpdateHandler().getSolrCoreState().newIndexWriter(core, false);
@@ -805,6 +792,24 @@ public SolrCore reload(ConfigSet coreConfig) throws IOException {
  }
  }
 
+ /**
+ * Clones the current core for core reload, with the provided CoreDescriptor and ConfigSet.
+ *
+ * @return the cloned core to be used for {@link SolrCore#reload}
+ */
+ protected SolrCore cloneForReloadCore(
+ CoreDescriptor newCoreDescriptor, ConfigSet newCoreConfig, boolean cloneCoreState) {
+ return new SolrCore(
+ coreContainer,
+ newCoreDescriptor,
+ newCoreConfig,
+ getDataDir(),
+ updateHandler,
+ solrDelPolicy,
+ cloneCoreState ? this : null,
+ true);
+ }
+
  private DirectoryFactory initDirectoryFactory() {
  return DirectoryFactory.loadDirectoryFactory(
  solrConfig, coreContainer, coreMetricManager.getRegistryName());
@@ -1054,7 +1059,7 @@ public CoreContainer getCoreContainer() {
  this(coreContainer, cd, configSet, null, null, null, null, false);
  }
 
- private SolrCore(
+ protected SolrCore(
  CoreContainer coreContainer,
  CoreDescriptor coreDescriptor,
  ConfigSet configSet,

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

@@ -22,6 +22,7 @@
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.params.CoreAdminParams;
 import org.apache.solr.rest.RestManager;
+import org.apache.solr.update.UpdateHandler;
 
 /**
  * A synthetic core that is created only in memory and not registered against Zookeeper.
@@ -34,7 +35,20 @@
  */
 public class SyntheticSolrCore extends SolrCore {
  public SyntheticSolrCore(CoreContainer coreContainer, CoreDescriptor cd, ConfigSet configSet) {
- super(coreContainer, cd, configSet);
+ this(coreContainer, cd, configSet, null, null, null, null, false);
+ }
+
+ public SyntheticSolrCore(
+ CoreContainer coreContainer,
+ CoreDescriptor coreDescriptor,
+ ConfigSet configSet,
+ String dataDir,
+ UpdateHandler updateHandler,
+ IndexDeletionPolicyWrapper delPolicy,
+ SolrCore prev,
+ boolean reload) {
+ super(
+ coreContainer, coreDescriptor, configSet, dataDir, updateHandler, delPolicy, prev, reload);
  }
 
  public static SyntheticSolrCore createAndRegisterCore(
@@ -72,4 +86,18 @@ protected RestManager initRestManager() throws SolrException {
  // We do not expect RestManager ops on Coordinator Nodes
  return new RestManager();
  }
+
+ @Override
+ protected SyntheticSolrCore cloneForReloadCore(
+ CoreDescriptor newCoreDescriptor, ConfigSet newCoreConfig, boolean cloneCurrentState) {
+ return new SyntheticSolrCore(
+ getCoreContainer(),
+ newCoreDescriptor,
+ newCoreConfig,
+ getDataDir(),
+ getUpdateHandler(),
+ getDeletionPolicy(),
+ cloneCurrentState ? this : null,
+ true);
+ }
 }

solr/core/src/test/org/apache/solr/search/TestCoordinatorRole.java

@@ -863,4 +863,67 @@ public void testMoveReplica() throws Exception {
  cluster.shutdown();
  }
  }
+
+ public void testCoreReload() throws Exception {
+ final int DATA_NODE_COUNT = 1;
+ MiniSolrCloudCluster cluster =
+ configureCluster(DATA_NODE_COUNT)
+ .addConfig("conf1", configset("cloud-minimal"))
+ .configure();
+
+ List<String> dataNodes =
+ cluster.getJettySolrRunners().stream()
+ .map(JettySolrRunner::getNodeName)
+ .collect(Collectors.toUnmodifiableList());
+
+ try {
+ CollectionAdminRequest.createCollection("c1", "conf1", 1, 1).process(cluster.getSolrClient());
+ cluster.waitForActiveCollection("c1", 1, 1);
+
+ System.setProperty(NodeRoles.NODE_ROLES_PROP, "coordinator:on");
+ JettySolrRunner coordinatorJetty;
+ try {
+ coordinatorJetty = cluster.startJettySolrRunner();
+ } finally {
+ System.clearProperty(NodeRoles.NODE_ROLES_PROP);
+ }
+
+ try (HttpSolrClient coordinatorClient =
+ new HttpSolrClient.Builder(coordinatorJetty.getBaseUrl().toString()).build()) {
+ HttpResponse response =
+ coordinatorClient
+ .getHttpClient()
+ .execute(
+ new HttpGet(
+ coordinatorJetty.getBaseUrl()
+ + "/c1/select?q:*:*")); // make a call so the synthetic core would be
+ // created
+ assertEquals(200, response.getStatusLine().getStatusCode());
+ // conf1 has no cache-control
+ assertNull(response.getFirstHeader("cache-control"));
+
+ // now update conf1
+ cluster.uploadConfigSet(configset("cache-control"), "conf1");
+
+ response =
+ coordinatorClient
+ .getHttpClient()
+ .execute(
+ new HttpGet(
+ coordinatorJetty.getBaseUrl()
+ + "/admin/cores?core=.sys.COORDINATOR-COLL-conf1_core&action=reload"));
+ assertEquals(200, response.getStatusLine().getStatusCode());
+
+ response =
+ coordinatorClient
+ .getHttpClient()
+ .execute(new HttpGet(coordinatorJetty.getBaseUrl() + "/c1/select?q:*:*"));
+ assertEquals(200, response.getStatusLine().getStatusCode());
+ // now the response should show cache-control
+ assertTrue(response.getFirstHeader("cache-control").getValue().contains("max-age=30"));
+ }
+ } finally {
+ cluster.shutdown();
+ }
+ }
 }