SOLR-16336: avoid fetching solrconfig.xml & schema.xml for already cached schema and config

solr/CHANGES.txt

@@ -80,6 +80,8 @@ Optimizations
 
 * SOLR-9359: Enable warming queries to be managed using Config API (Andy Webb, Eric Pugh)
 
+* SOLR-16336: avoid fetching solrconfig.xml & schema.xml for already cached schema and config (noble)
+
 Bug Fixes
 ---------------------
 * SOLR-15918: Skip repetitive parent znode creation on config set upload (Mike Drob)

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

@@ -24,6 +24,7 @@
 import java.nio.file.Path;
 import org.apache.solr.common.SolrException.ErrorCode;
 import org.apache.solr.common.cloud.ZooKeeperException;
+import org.apache.solr.common.util.Pair;
 import org.apache.solr.core.CoreContainer;
 import org.apache.solr.core.SolrResourceLoader;
 import org.apache.solr.core.SolrResourceNotFoundException;
@@ -54,6 +55,20 @@ public ZkSolrResourceLoader(
     configSetZkPath = ZkConfigSetService.CONFIGS_ZKNODE + "/" + configSet;
   }
 
+  public Pair<String, Integer> getZkResourceInfo(String resource) {
+    String file = (".".equals(resource)) ? configSetZkPath : configSetZkPath + "/" + resource;
+    try {
+      Stat stat = zkController.getZkClient().exists(file, null, true);
+      if (stat != null) {
+        return new Pair<>(file, stat.getVersion());
+      } else {
+        return null;
+      }
+    } catch (Exception e) {
+      return null;
+    }
+  }
+
   /**
    * Opens any resource by its name. By default, this will look in multiple locations to load the
    * resource: $configDir/$resource from ZooKeeper. It will look for it in any jar accessible

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

@@ -49,7 +49,6 @@
 import java.util.Properties;
 import java.util.Set;
 import java.util.UUID;
-import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.function.Function;
 import java.util.function.Predicate;
@@ -173,8 +172,8 @@ private class ResourceProvider implements Function<String, InputStream> {
     InputStream in;
     String fileName;
 
-    ResourceProvider(InputStream in) {
-      this.in = in;
+    ResourceProvider(SolrResourceLoader loader, String res) throws IOException {
+      this.in = loader.openResource(res);
       if (in instanceof ZkSolrResourceLoader.ZkByteArrayInputStream) {
         ZkSolrResourceLoader.ZkByteArrayInputStream zkin =
             (ZkSolrResourceLoader.ZkByteArrayInputStream) in;
@@ -213,33 +212,19 @@ private SolrConfig(
     this.substituteProperties = substitutableProperties;
     getOverlay(); // just in case it is not initialized
     // insist we have non-null substituteProperties; it might get overlaid
-    Map<String, IndexSchemaFactory.VersionedConfig> configCache = null;
-    if (loader.getCoreContainer() != null && loader.getCoreContainer().getObjectCache() != null) {
-      configCache =
-          (Map<String, IndexSchemaFactory.VersionedConfig>)
-              loader
-                  .getCoreContainer()
-                  .getObjectCache()
-                  .computeIfAbsent(
-                      ConfigSetService.ConfigResource.class.getName(),
-                      s -> new ConcurrentHashMap<>());
-      ResourceProvider rp = new ResourceProvider(loader.openResource(name));
-      IndexSchemaFactory.VersionedConfig cfg =
-          rp.fileName == null ? null : configCache.get(rp.fileName);
-      if (cfg != null) {
-        if (rp.hash != -1) {
-          if (rp.hash == cfg.version) {
-            log.debug("LOADED_FROM_CACHE");
-            root = cfg.data;
-          } else {
-            readXml(loader, name, configCache, rp);
-          }
-        }
-      }
-    }
-    if (root == null) {
-      readXml(loader, name, configCache, new ResourceProvider(loader.openResource(name)));
-    }
+
+    IndexSchemaFactory.VersionedConfig cfg =
+        IndexSchemaFactory.getFromCache(
+            name,
+            loader,
+            () -> {
+              if (loader.getCoreContainer() == null) return null;
+              return loader.getCoreContainer().getObjectCache();
+            },
+            () -> readXml(loader, name));
+    this.root = cfg.data;
+    this.znodeVersion = cfg.version;
+
     ConfigNode.SUBSTITUTES.set(
         key -> {
           if (substitutableProperties != null && substitutableProperties.containsKey(key)) {
@@ -407,17 +392,15 @@ private SolrConfig(
     }
   }
 
-  private void readXml(
-      SolrResourceLoader loader,
-      String name,
-      Map<String, IndexSchemaFactory.VersionedConfig> configCache,
-      ResourceProvider rp)
-      throws IOException {
-    XmlConfigFile xml = new XmlConfigFile(loader, rp, name, null, "/config/", null);
-    root = new DataConfigNode(new DOMConfigNode(xml.getDocument().getDocumentElement()));
-    this.znodeVersion = rp.zkVersion;
-    if (configCache != null && rp.fileName != null) {
-      configCache.put(rp.fileName, new IndexSchemaFactory.VersionedConfig(rp.hash, root));
+  private IndexSchemaFactory.VersionedConfig readXml(SolrResourceLoader loader, String name) {
+    try {
+      ResourceProvider rp = new ResourceProvider(loader, name);
+      XmlConfigFile xml = new XmlConfigFile(loader, rp, name, null, "/config/", null);
+      return new IndexSchemaFactory.VersionedConfig(
+          rp.zkVersion,
+          new DataConfigNode(new DOMConfigNode(xml.getDocument().getDocumentElement())));
+    } catch (IOException e) {
+      throw new SolrException(ErrorCode.SERVER_ERROR, e);
     }
   }
 

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

@@ -23,12 +23,16 @@
 import java.lang.invoke.MethodHandles;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Consumer;
+import java.util.function.Supplier;
 import javax.xml.parsers.ParserConfigurationException;
 import org.apache.solr.cloud.ZkConfigSetService;
 import org.apache.solr.cloud.ZkSolrResourceLoader;
 import org.apache.solr.common.ConfigNode;
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.SolrException.ErrorCode;
+import org.apache.solr.common.util.ObjectCache;
+import org.apache.solr.common.util.Pair;
 import org.apache.solr.core.ConfigSetService;
 import org.apache.solr.core.PluginInfo;
 import org.apache.solr.core.SolrConfig;
@@ -89,16 +93,14 @@ public String getSchemaResourceName(String cdResourceName) {
   public IndexSchema create(
       String resourceName, SolrConfig config, ConfigSetService configSetService) {
     SolrResourceLoader loader = config.getResourceLoader();
-    InputStream schemaInputStream = null;
 
     if (null == resourceName) {
       resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
     }
     try {
-      schemaInputStream = loader.openResource(resourceName);
       return new IndexSchema(
           resourceName,
-          getConfigResource(configSetService, schemaInputStream, loader, resourceName),
+          getConfigResource(configSetService, null, loader, resourceName),
           config.luceneMatchVersion,
           loader,
           config.getSubstituteProperties());
@@ -111,43 +113,80 @@ public IndexSchema create(
     }
   }
 
-  @SuppressWarnings("unchecked")
   public static ConfigSetService.ConfigResource getConfigResource(
       ConfigSetService configSetService,
       InputStream schemaInputStream,
       SolrResourceLoader loader,
-      String name)
-      throws IOException {
-    if (configSetService instanceof ZkConfigSetService
-        && schemaInputStream instanceof ZkSolrResourceLoader.ZkByteArrayInputStream) {
-      ZkSolrResourceLoader.ZkByteArrayInputStream is =
-          (ZkSolrResourceLoader.ZkByteArrayInputStream) schemaInputStream;
-      Map<String, VersionedConfig> configCache =
+      String name) {
+    return () ->
+        getFromCache(
+                name,
+                loader,
+                () -> {
+                  if (!(configSetService instanceof ZkConfigSetService)) return null;
+                  return ((ZkConfigSetService) configSetService)
+                      .getSolrCloudManager()
+                      .getObjectCache();
+                },
+                () -> loadConfig(schemaInputStream, loader, name))
+            .data;
+  }
+
+  private static VersionedConfig loadConfig(
+      InputStream schemaInputStream, SolrResourceLoader loader, String name) {
+    try {
+      InputStream is = (schemaInputStream == null ? loader.openResource(name) : schemaInputStream);
+      ConfigNode node = getParsedSchema(is, loader, name);
+      int version =
+          is instanceof ZkSolrResourceLoader.ZkByteArrayInputStream
+              ? ((ZkSolrResourceLoader.ZkByteArrayInputStream) is).getStat().getVersion()
+              : 0;
+      return new VersionedConfig(version, node);
+    } catch (Exception e) {
+      throw new SolrException(ErrorCode.SERVER_ERROR, "Error fetching schema", e);
+    }
+  }
+
+  // for testing purposes
+  public static volatile Consumer<String> CACHE_MISS_LISTENER = null;
+
+  @SuppressWarnings("unchecked")
+  public static VersionedConfig getFromCache(
+      String name,
+      SolrResourceLoader loader,
+      Supplier<ObjectCache> objectCacheSupplier,
+      Supplier<VersionedConfig> c) {
+    Consumer<String> listener = CACHE_MISS_LISTENER;
+    Supplier<VersionedConfig> cfgLoader =
+        listener == null
+            ? c
+            : () -> {
+              listener.accept(name);
+              return c.get();
+            };
+
+    if (loader instanceof ZkSolrResourceLoader) {
+      ZkSolrResourceLoader zkLoader = (ZkSolrResourceLoader) loader;
+      ObjectCache objectCache = objectCacheSupplier.get();
+      if (objectCache == null) return cfgLoader.get();
+      Map<String, VersionedConfig> confCache =
           (Map<String, VersionedConfig>)
-              ((ZkConfigSetService) configSetService)
-                  .getSolrCloudManager()
-                  .getObjectCache()
-                  .computeIfAbsent(
-                      ConfigSetService.ConfigResource.class.getName(),
-                      s -> new ConcurrentHashMap<>());
-      VersionedConfig cached = configCache.get(is.fileName);
-      if (cached != null) {
-        if (cached.version != is.getStat().getVersion()) {
-          configCache.remove(is.fileName); // this is stale. remove from cache
-        } else {
-          return () -> cached.data;
-        }
+              objectCache.computeIfAbsent(
+                  ConfigSetService.ConfigResource.class.getName(), k -> new ConcurrentHashMap<>());
+      Pair<String, Integer> res = zkLoader.getZkResourceInfo(name);
+      if (res == null) return cfgLoader.get();
+      VersionedConfig result = null;
+      result = confCache.computeIfAbsent(res.first(), k -> cfgLoader.get());
+      if (result.version == res.second()) {
+        return result;
+      } else {
+        confCache.remove(res.first());
+        return confCache.computeIfAbsent(res.first(), k -> cfgLoader.get());
       }
-      return () -> {
-        ConfigNode data =
-            getParsedSchema(
-                schemaInputStream, loader, name); // either missing or stale. create a new one
-        configCache.put(is.fileName, new VersionedConfig(is.getStat().getVersion(), data));
-        return data;
-      };
+    } else {
+      // it's a file system loader, no caching necessary
+      return cfgLoader.get();
     }
-    // this is not cacheable as it does not come from ZK
-    return () -> getParsedSchema(schemaInputStream, loader, name);
   }
 
   public static ConfigNode getParsedSchema(InputStream is, SolrResourceLoader loader, String name)

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

@@ -277,7 +277,7 @@ public ManagedIndexSchema create(
               managedSchemaResourceName,
               schemaZkVersion,
               getSchemaUpdateLock());
-    } catch (IOException e) {
+    } catch (Exception e) {
       throw new SolrException(ErrorCode.SERVER_ERROR, "Error loading parsing schema", e);
     }
     if (shouldUpgrade) {

solr/core/src/test/org/apache/solr/cloud/api/collections/TestCollectionAPI.java

@@ -25,6 +25,7 @@
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.atomic.LongAdder;
 import org.apache.solr.client.solrj.SolrRequest;
 import org.apache.solr.client.solrj.SolrResponse;
 import org.apache.solr.client.solrj.SolrServerException;
@@ -35,6 +36,7 @@
 import org.apache.solr.client.solrj.request.QueryRequest;
 import org.apache.solr.client.solrj.request.V2Request;
 import org.apache.solr.client.solrj.response.CollectionAdminResponse;
+import org.apache.solr.cloud.MiniSolrCloudCluster;
 import org.apache.solr.cloud.ZkConfigSetService;
 import org.apache.solr.cloud.ZkTestServer;
 import org.apache.solr.common.SolrException;
@@ -50,6 +52,7 @@
 import org.apache.solr.common.params.ShardParams;
 import org.apache.solr.common.util.NamedList;
 import org.apache.solr.common.util.Utils;
+import org.apache.solr.schema.IndexSchemaFactory;
 import org.apache.zookeeper.KeeperException;
 import org.junit.Test;
 
@@ -1279,4 +1282,28 @@ public void testRecreateCollectionAfterFailure() throws Exception {
       assertSame(0, rsp.getStatus());
     }
   }
+
+  @Test
+  public void testConfigCaching() throws Exception {
+    String COLL = "cfg_cache_test";
+    MiniSolrCloudCluster cl =
+        new MiniSolrCloudCluster.Builder(2, createTempDir())
+            .addConfig("conf", configset("cloud-minimal"))
+            .configure();
+
+    try {
+      LongAdder schemaMisses = new LongAdder();
+      LongAdder configMisses = new LongAdder();
+      IndexSchemaFactory.CACHE_MISS_LISTENER =
+          s -> {
+            if ("schema.xml".equals(s)) schemaMisses.increment();
+            if ("solrconfig.xml".equals(s)) configMisses.increment();
+          };
+      CollectionAdminRequest.createCollection(COLL, "conf", 5, 1).process(cl.getSolrClient());
+      assertEquals(2, schemaMisses.longValue());
+      assertEquals(2, configMisses.longValue());
+    } finally {
+      cl.shutdown();
+    }
+  }
 }