SOLR-9088: Fixed TestManagedSchemaAPI failures which exposed race conditions in the schema API

solr/CHANGES.txt

@@ -117,6 +117,8 @@ Bug Fixes
 
 * SOLR-9235: Fixed NPE when using non-numeric range query in deleteByQuery (hossman)
 
+* SOLR-9088: Fixed TestManagedSchemaAPI failures which exposed race conditions in the schema API ( Varun Thacker, noble)
+
 Optimizations
 ----------------------
 

solr/core/src/java/org/apache/solr/cloud/ZkController.java

@@ -53,7 +53,25 @@ import org.apache.solr.cloud.overseer.OverseerAction;
 import org.apache.solr.cloud.overseer.SliceMutator;
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.SolrException.ErrorCode;
-import org.apache.solr.common.cloud.*;
+import org.apache.solr.common.cloud.BeforeReconnect;
+import org.apache.solr.common.cloud.ClusterState;
+import org.apache.solr.common.cloud.ClusterStateUtil;
+import org.apache.solr.common.cloud.DefaultConnectionStrategy;
+import org.apache.solr.common.cloud.DefaultZkACLProvider;
+import org.apache.solr.common.cloud.DefaultZkCredentialsProvider;
+import org.apache.solr.common.cloud.DocCollection;
+import org.apache.solr.common.cloud.OnReconnect;
+import org.apache.solr.common.cloud.Replica;
+import org.apache.solr.common.cloud.Slice;
+import org.apache.solr.common.cloud.SolrZkClient;
+import org.apache.solr.common.cloud.ZkACLProvider;
+import org.apache.solr.common.cloud.ZkCmdExecutor;
+import org.apache.solr.common.cloud.ZkConfigManager;
+import org.apache.solr.common.cloud.ZkCoreNodeProps;
+import org.apache.solr.common.cloud.ZkCredentialsProvider;
+import org.apache.solr.common.cloud.ZkNodeProps;
+import org.apache.solr.common.cloud.ZkStateReader;
+import org.apache.solr.common.cloud.ZooKeeperException;
 import org.apache.solr.common.params.CollectionParams;
 import org.apache.solr.common.params.CommonParams;
 import org.apache.solr.common.params.SolrParams;
@@ -2242,8 +2260,8 @@ public final class ZkController {
     String errMsg = "Failed to persist resource at {0} - old {1}";
     try {
       try {
-        zkClient.setData(resourceLocation, content, znodeVersion, true);
+        Stat stat = zkClient.setData(resourceLocation, content, znodeVersion, true);
-        latestVersion = znodeVersion + 1;// if the set succeeded , it should have incremented the version by one always
+        latestVersion = stat.getVersion();// if the set succeeded , it should have incremented the version by one always
         log.info("Persisted config data to node {} ", resourceLocation);
         touchConfDir(zkLoader);
       } catch (NoNodeException e) {

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

@@ -28,7 +28,19 @@ import java.lang.reflect.Constructor;
 import java.net.URL;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.NoSuchFileException;
-import java.util.*;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Date;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.Properties;
+import java.util.Set;
 import java.util.concurrent.Callable;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.CopyOnWriteArrayList;
@@ -77,7 +89,22 @@ import org.apache.solr.handler.component.SearchComponent;
 import org.apache.solr.logging.MDCLoggingContext;
 import org.apache.solr.request.SolrQueryRequest;
 import org.apache.solr.request.SolrRequestHandler;
-import org.apache.solr.response.*;
+import org.apache.solr.response.BinaryResponseWriter;
+import org.apache.solr.response.CSVResponseWriter;
+import org.apache.solr.response.GeoJSONResponseWriter;
+import org.apache.solr.response.GraphMLResponseWriter;
+import org.apache.solr.response.JSONResponseWriter;
+import org.apache.solr.response.PHPResponseWriter;
+import org.apache.solr.response.PHPSerializedResponseWriter;
+import org.apache.solr.response.PythonResponseWriter;
+import org.apache.solr.response.QueryResponseWriter;
+import org.apache.solr.response.RawResponseWriter;
+import org.apache.solr.response.RubyResponseWriter;
+import org.apache.solr.response.SchemaXmlResponseWriter;
+import org.apache.solr.response.SmileResponseWriter;
+import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.response.SortingResponseWriter;
+import org.apache.solr.response.XMLResponseWriter;
 import org.apache.solr.response.transform.TransformerFactory;
 import org.apache.solr.rest.ManagedResourceStorage;
 import org.apache.solr.rest.ManagedResourceStorage.StorageIO;
@@ -86,6 +113,7 @@ import org.apache.solr.schema.FieldType;
 import org.apache.solr.schema.IndexSchema;
 import org.apache.solr.schema.IndexSchemaFactory;
 import org.apache.solr.schema.ManagedIndexSchema;
+import org.apache.solr.schema.SchemaManager;
 import org.apache.solr.schema.SimilarityFactory;
 import org.apache.solr.search.QParserPlugin;
 import org.apache.solr.search.SolrFieldCacheMBean;
@@ -2488,13 +2516,13 @@ public final class SolrCore implements SolrInfoMBean, Closeable {
       SolrZkClient zkClient = cc.getZkController().getZkClient();
       int solrConfigversion, overlayVersion, managedSchemaVersion = 0;
       SolrConfig cfg = null;
-      try (SolrCore core1 = cc.solrCores.getCoreFromAnyList(coreName, true)) {
+      try (SolrCore solrCore = cc.solrCores.getCoreFromAnyList(coreName, true)) {
-        if (core1 == null || core1.isClosed()) return;
+        if (solrCore == null || solrCore.isClosed()) return;
-        cfg = core1.getSolrConfig();
+        cfg = solrCore.getSolrConfig();
-        solrConfigversion = core1.getSolrConfig().getOverlay().getZnodeVersion();
+        solrConfigversion = solrCore.getSolrConfig().getOverlay().getZnodeVersion();
-        overlayVersion = core1.getSolrConfig().getZnodeVersion();
+        overlayVersion = solrCore.getSolrConfig().getZnodeVersion();
         if (managedSchmaResourcePath != null) {
-          managedSchemaVersion = ((ManagedIndexSchema) core1.getLatestSchema()).getSchemaZkVersion();
+          managedSchemaVersion = ((ManagedIndexSchema) solrCore.getLatestSchema()).getSchemaZkVersion();
         }
 
       }
@@ -2504,6 +2532,13 @@ public final class SolrCore implements SolrInfoMBean, Closeable {
       if (checkStale(zkClient, overlayPath, solrConfigversion) ||
           checkStale(zkClient, solrConfigPath, overlayVersion) ||
           checkStale(zkClient, managedSchmaResourcePath, managedSchemaVersion)) {
+
+        try (SolrCore solrCore = cc.solrCores.getCoreFromAnyList(coreName, true)) {
+          solrCore.setLatestSchema(SchemaManager.getFreshManagedSchema(solrCore));
+        } catch (Exception e) {
+          log.warn("", SolrZkClient.checkInterrupted(e));
+        }
+
         log.info("core reload {}", coreName);
         try {
           cc.reload(coreName);
@@ -2513,9 +2548,9 @@ public final class SolrCore implements SolrInfoMBean, Closeable {
         return;
       }
       //some files in conf directory may have  other than managedschema, overlay, params
-      try (SolrCore core1 = cc.solrCores.getCoreFromAnyList(coreName, true)) {
+      try (SolrCore solrCore = cc.solrCores.getCoreFromAnyList(coreName, true)) {
-        if (core1 == null || core1.isClosed()) return;
+        if (solrCore == null || solrCore.isClosed()) return;
-        for (Runnable listener : core1.confListeners) {
+        for (Runnable listener : solrCore.confListeners) {
           try {
             listener.run();
           } catch (Exception e) {

solr/core/src/java/org/apache/solr/schema/SchemaManager.java

@@ -16,6 +16,18 @@
  */
 package org.apache.solr.schema;
 
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.Reader;
+import java.io.StringWriter;
+import java.lang.invoke.MethodHandles;
+import java.nio.charset.StandardCharsets;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+
 import org.apache.solr.cloud.ZkController;
 import org.apache.solr.cloud.ZkSolrResourceLoader;
 import org.apache.solr.common.SolrException;
@@ -31,18 +43,6 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.xml.sax.InputSource;
 
-import java.io.IOException;
-import java.io.InputStream;
-import java.io.Reader;
-import java.io.StringWriter;
-import java.lang.invoke.MethodHandles;
-import java.nio.charset.StandardCharsets;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.concurrent.TimeUnit;
-
 import static java.util.Collections.singleton;
 import static java.util.Collections.singletonList;
 import static java.util.Collections.singletonMap;
@@ -108,7 +108,7 @@ public class SchemaManager {
     SolrCore core = req.getCore();
     String errorMsg = "Unable to persist managed schema. ";
     while (!timeOut.hasTimedOut()) {
-      managedIndexSchema = getFreshManagedSchema();
+      managedIndexSchema = getFreshManagedSchema(req.getCore());
       for (CommandOperation op : operations) {
         OpType opType = OpType.get(op.name);
         if (opType != null) {
@@ -131,9 +131,9 @@ public class SchemaManager {
         }
 
         try {
-          ZkController.persistConfigResourceToZooKeeper(zkLoader, managedIndexSchema.getSchemaZkVersion(),
+          int latestVersion = ZkController.persistConfigResourceToZooKeeper(zkLoader, managedIndexSchema.getSchemaZkVersion(),
               managedIndexSchema.getResourceName(), sw.toString().getBytes(StandardCharsets.UTF_8), true);
-          waitForOtherReplicasToUpdate(timeOut);
+          waitForOtherReplicasToUpdate(timeOut, latestVersion);
           core.setLatestSchema(managedIndexSchema);
           return Collections.emptyList();
         } catch (ZkController.ResourceModifiedInZkException e) {
@@ -155,7 +155,7 @@ public class SchemaManager {
     return singletonList(errorMsg + "Timed out.");
   }
 
-  private void waitForOtherReplicasToUpdate(TimeOut timeOut) {
+  private void waitForOtherReplicasToUpdate(TimeOut timeOut, int latestVersion) {
     CoreDescriptor cd = req.getCore().getCoreDescriptor();
     String collection = cd.getCollectionName();
     if (collection != null) {
@@ -164,11 +164,8 @@ public class SchemaManager {
         throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
             "Not enough time left to update replicas. However, the schema is updated already.");
       }
-      ManagedIndexSchema.waitForSchemaZkVersionAgreement(collection,
+      ManagedIndexSchema.waitForSchemaZkVersionAgreement(collection, cd.getCloudDescriptor().getCoreNodeName(),
-          cd.getCloudDescriptor().getCoreNodeName(),
+          latestVersion, zkLoader.getZkController(), (int) timeOut.timeLeft(TimeUnit.SECONDS));
-          (managedIndexSchema).getSchemaZkVersion(),
-          zkLoader.getZkController(),
-          (int) timeOut.timeLeft(TimeUnit.SECONDS));
     }
   }
 
@@ -423,21 +420,23 @@ public class SchemaManager {
     return sb.toString();
   }
 
-  public ManagedIndexSchema getFreshManagedSchema() throws IOException, KeeperException, InterruptedException {
+  public static ManagedIndexSchema getFreshManagedSchema(SolrCore core) throws IOException,
-    SolrResourceLoader resourceLoader = req.getCore().getResourceLoader();
+      KeeperException, InterruptedException {
+
+    SolrResourceLoader resourceLoader = core.getResourceLoader();
+    String name = core.getLatestSchema().getResourceName();
     if (resourceLoader instanceof ZkSolrResourceLoader) {
-      InputStream in = resourceLoader.openResource(req.getSchema().getResourceName());
+      InputStream in = resourceLoader.openResource(name);
       if (in instanceof ZkSolrResourceLoader.ZkByteArrayInputStream) {
         int version = ((ZkSolrResourceLoader.ZkByteArrayInputStream) in).getStat().getVersion();
         log.info("managed schema loaded . version : {} ", version);
-        return new ManagedIndexSchema
+        return new ManagedIndexSchema(core.getSolrConfig(), name, new InputSource(in), true, name, version,
-            (req.getCore().getSolrConfig(), req.getSchema().getResourceName(), new InputSource(in),
+            core.getLatestSchema().getSchemaUpdateLock());
-                true, req.getSchema().getResourceName(), version, req.getSchema().getSchemaUpdateLock());
       } else {
-        return (ManagedIndexSchema) req.getCore().getLatestSchema();
+        return (ManagedIndexSchema) core.getLatestSchema();
       }
     } else {
-      return (ManagedIndexSchema) req.getCore().getLatestSchema();
+      return (ManagedIndexSchema) core.getLatestSchema();
     }
   }
 }