SOLR-18378: remove CoreContainer.getCores() and SolrCores.getCores(), 26 sites

changelog/unreleased/SOLR-18378-remove-corecontainer-solrcores-getcores.yml

@@ -0,0 +1,8 @@
+# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
+title: Remove the deprecated CoreContainer.getCores() and SolrCores.getCores() methods, which handed out cores without reserving them. Use getLoadedCoreNames() with getCore(String) and close each core, or getLoadedCoreNames().size() when only a count is needed.
+type: removed
+authors:
+  - name: Serhiy Bzhezytskyy
+links:
+  - name: SOLR-18378
+    url: https://issues.apache.org/jira/browse/SOLR-18378

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

@@ -58,6 +58,7 @@
 import java.util.concurrent.Executor;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.TimeoutException;
+import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
@@ -1381,21 +1382,32 @@ public void shutdown() {
     metricManager.closeAllRegistries(); // Close all OTEL meter providers and metrics
   }
 
-  public void cancelCoreRecoveries() {
-
-    List<SolrCore> cores = solrCores.getCores();
-
-    // we must cancel without holding the cores sync
-    // make sure we wait for any recoveries to stop
-    for (SolrCore core : cores) {
-      try {
-        core.getSolrCoreState().cancelRecovery();
-      } catch (Exception e) {
-        log.error("Error canceling recovery for core", e);
+  /**
+   * Applies the given action to each currently loaded core, reserving and releasing each one
+   * without triggering a lazy load. A core unloaded concurrently after {@link
+   * #getLoadedCoreNames()} was called is skipped.
+   */
+  public void forEachLoadedCore(Consumer<SolrCore> action) {
+    for (String coreName : solrCores.getLoadedCoreNames()) {
+      // getCoreFromAnyList, not getCore: never loads
+      try (SolrCore core = solrCores.getCoreFromAnyList(coreName, true)) {
+        if (core == null) continue; // unloaded since getLoadedCoreNames
+        action.accept(core);
       }
     }
   }
 
+  public void cancelCoreRecoveries() {
+    forEachLoadedCore(
+        core -> {
+          try {
+            core.getSolrCoreState().cancelRecovery();
+          } catch (Exception e) {
+            log.error("Error canceling recovery for core", e);
+          }
+        });
+  }
+
   /**
    * Pause updates for all cores on this node and wait for all in-flight update requests to finish.
    * Here, we (slightly) delay leader election so that in-flight update requests succeed and we can
@@ -1408,21 +1420,25 @@ public void cancelCoreRecoveries() {
    * <p>We do not need to unpause ever because the node is being shut down.
    */
   private void pauseUpdatesAndAwaitInflightRequests() {
-    getCores().parallelStream()
+    solrCores.getLoadedCoreNames().parallelStream()
         .forEach(
-            solrCore -> {
-              SolrCoreState solrCoreState = solrCore.getSolrCoreState();
-              try {
-                solrCoreState.pauseUpdatesAndAwaitInflightRequests();
-              } catch (TimeoutException e) {
-                log.warn(
-                    "Timed out waiting for in-flight update requests to complete for core: {}",
-                    solrCore.getName());
-              } catch (InterruptedException e) {
-                log.warn(
-                    "Interrupted while waiting for in-flight update requests to complete for core: {}",
-                    solrCore.getName());
-                Thread.currentThread().interrupt();
+            coreName -> {
+              // see cancelCoreRecoveries: reserve without loading, we are shutting down
+              try (SolrCore solrCore = solrCores.getCoreFromAnyList(coreName, true)) {
+                if (solrCore == null) return; // unloaded since getLoadedCoreNames
+                SolrCoreState solrCoreState = solrCore.getSolrCoreState();
+                try {
+                  solrCoreState.pauseUpdatesAndAwaitInflightRequests();
+                } catch (TimeoutException e) {
+                  log.warn(
+                      "Timed out waiting for in-flight update requests to complete for core: {}",
+                      solrCore.getName());
+                } catch (InterruptedException e) {
+                  log.warn(
+                      "Interrupted while waiting for in-flight update requests to complete for core: {}",
+                      solrCore.getName());
+                  Thread.currentThread().interrupt();
+                }
               }
             });
   }
@@ -1838,21 +1854,6 @@ private void resetIndexDirectory(CoreDescriptor dcore, ConfigSet coreConfig) {
     }
   }
 
-  /**
-   * Gets all loaded cores, consistent with {@link #getLoadedCoreNames()}. Caller doesn't need to
-   * close.
-   *
-   * <p>NOTE: rather dangerous API because each core is not reserved (could in theory be closed).
-   * Prefer {@link #getLoadedCoreNames()} and then call {@link #getCore(String)} then close it.
-   *
-   * @return An unsorted list. This list is a new copy, it can be modified by the caller (e.g. it
-   *     can be sorted). Don't need to close them.
-   */
-  @Deprecated
-  public List<SolrCore> getCores() {
-    return solrCores.getCores();
-  }
-
   /**
    * Gets the cores that are currently loaded, i.e. cores that have 1: loadOnStartup=true and have
    * been loaded and 2: loadOnStartup=false and have been subsequently loaded.

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

@@ -142,20 +142,6 @@ public SolrCore putCore(CoreDescriptor cd, SolrCore core) {
     }
   }
 
-  /**
-   * @return A list of "permanent" cores, i.e. cores that may not be swapped out and are currently
-   *     loaded.
-   *     <p>A core may be non-transient but still lazily loaded. If it is "permanent" and lazy-load
-   *     _and_ not yet loaded it will _not_ be returned by this call.
-   *     <p>This list is a new copy, it can be modified by the caller (e.g. it can be sorted).
-   */
-  @Deprecated
-  public List<SolrCore> getCores() {
-    synchronized (modifyLock) {
-      return new ArrayList<>(cores.values());
-    }
-  }
-
   /**
    * Gets the cores that are currently loaded, i.e. cores that have 1: loadOnStartup=true and are
    * either not-transient or, if transient, have been loaded and have not been aged out 2:
@@ -189,7 +175,7 @@ public List<String> getAllCoreNames() {
 
   /**
    * Gets the number of currently loaded permanent (non transient) cores. Faster equivalent for
-   * {@link #getCores()}.size().
+   * {@link #getLoadedCoreNames()}.size().
    */
   public int getNumLoadedPermanentCores() {
     synchronized (modifyLock) {

solr/core/src/java/org/apache/solr/handler/admin/api/NodeHealth.java

@@ -151,13 +151,16 @@ private void healthCheckStandaloneMode(NodeHealthResponse response, Integer maxG
         return;
       }
 
-      for (SolrCore core : coreContainer.getCores()) {
-        ReplicationHandler replicationHandler =
-            (ReplicationHandler) core.getRequestHandler(ReplicationHandler.PATH);
-        if (replicationHandler.isFollower()) {
-          boolean isCoreInSync =
-              isWithinGenerationLag(core, replicationHandler, maxGenerationLag, laggingCoresInfo);
-          allCoresAreInSync &= isCoreInSync;
+      for (String coreName : coreContainer.getLoadedCoreNames()) {
+        try (SolrCore core = coreContainer.getCore(coreName)) {
+          if (core == null) continue; // unloaded since getLoadedCoreNames
+          ReplicationHandler replicationHandler =
+              (ReplicationHandler) core.getRequestHandler(ReplicationHandler.PATH);
+          if (replicationHandler.isFollower()) {
+            boolean isCoreInSync =
+                isWithinGenerationLag(core, replicationHandler, maxGenerationLag, laggingCoresInfo);
+            allCoresAreInSync &= isCoreInSync;
+          }
         }
       }
 

solr/core/src/java/org/apache/solr/pkg/SolrPackageLoader.java

@@ -106,8 +106,11 @@ public void refreshPackageConf() {
         }
       }
     }
-    for (SolrCore core : coreContainer.getCores()) {
-      core.getPackageListeners().packagesUpdated(updated);
+    for (String coreName : coreContainer.getLoadedCoreNames()) {
+      try (SolrCore core = coreContainer.getCore(coreName)) {
+        if (core == null) continue; // unloaded since getLoadedCoreNames
+        core.getPackageListeners().packagesUpdated(updated);
+      }
     }
     myCopy = packageAPI.pkgs;
   }
@@ -146,8 +149,11 @@ public void notifyListeners(String pkg) {
     SolrPackage p = packageClassLoaders.get(pkg);
     if (p != null) {
       List<SolrPackage> l = List.of(p);
-      for (SolrCore core : coreContainer.getCores()) {
-        core.getPackageListeners().packagesUpdated(l);
+      for (String coreName : coreContainer.getLoadedCoreNames()) {
+        try (SolrCore core = coreContainer.getCore(coreName)) {
+          if (core == null) continue; // unloaded since getLoadedCoreNames
+          core.getPackageListeners().packagesUpdated(l);
+        }
       }
     }
   }

solr/core/src/test/org/apache/solr/cloud/LeaderFailureAfterFreshStartTest.java

@@ -135,7 +135,7 @@ public void test() throws Exception {
 
     // start the freshNode
     restartNodes(List.of(freshNode));
-    String coreName = freshNode.jetty.getCoreContainer().getCores().iterator().next().getName();
+    String coreName = freshNode.jetty.getCoreContainer().getLoadedCoreNames().get(0);
     Path replicationProperties =
         Path.of(freshNode.jetty.getSolrHome(), "cores", coreName, "data", "replication.properties");
     String md5 = DigestUtils.md5Hex(Files.readAllBytes(replicationProperties));

solr/core/src/test/org/apache/solr/cloud/TestCloudRecovery.java

@@ -179,14 +179,18 @@ public void corruptedLogTest() throws Exception {
     int logHeaderSize = Integer.MAX_VALUE;
     Map<String, byte[]> contentFiles = new HashMap<>();
     for (JettySolrRunner solrRunner : cluster.getJettySolrRunners()) {
-      for (SolrCore solrCore : solrRunner.getCoreContainer().getCores()) {
-        Path tlogFolder = Path.of(solrCore.getUpdateHandler().getUpdateLog().getTlogDir());
-        try (Stream<Path> tLogFiles = Files.list(tlogFolder)) {
-          Path lastTLogFile =
-              tlogFolder.resolve(tLogFiles.sorted().toList().getLast().getFileName());
-          byte[] tlogBytes = Files.readAllBytes(lastTLogFile);
-          contentFiles.put(lastTLogFile.toString(), tlogBytes);
-          logHeaderSize = Math.min(tlogBytes.length, logHeaderSize);
+      CoreContainer coreContainer = solrRunner.getCoreContainer();
+      for (String coreName : coreContainer.getLoadedCoreNames()) {
+        try (SolrCore solrCore = coreContainer.getCore(coreName)) {
+          if (solrCore == null) continue; // unloaded since getLoadedCoreNames
+          Path tlogFolder = Path.of(solrCore.getUpdateHandler().getUpdateLog().getTlogDir());
+          try (Stream<Path> tLogFiles = Files.list(tlogFolder)) {
+            Path lastTLogFile =
+                tlogFolder.resolve(tLogFiles.sorted().toList().getLast().getFileName());
+            byte[] tlogBytes = Files.readAllBytes(lastTLogFile);
+            contentFiles.put(lastTLogFile.toString(), tlogBytes);
+            logHeaderSize = Math.min(tlogBytes.length, logHeaderSize);
+          }
         }
       }
     }

solr/core/src/test/org/apache/solr/cloud/TestPullReplicaErrorHandling.java

@@ -39,6 +39,7 @@
 import org.apache.solr.common.cloud.Slice;
 import org.apache.solr.common.cloud.ZkStateReader;
 import org.apache.solr.common.util.TimeSource;
+import org.apache.solr.core.CoreContainer;
 import org.apache.solr.core.SolrCore;
 import org.apache.solr.embedded.JettySolrRunner;
 import org.apache.solr.util.SocketProxy;
@@ -251,16 +252,19 @@ public void testCloseHooksDeletedOnReconnect() throws Exception {
     DocCollection docCollection = assertNumberOfReplicas(1, 0, 1, false, true);
     Slice s = docCollection.getSlices().iterator().next();
     JettySolrRunner jetty = getJettyForReplica(s.getReplicas(EnumSet.of(Replica.Type.PULL)).get(0));
-    SolrCore core = jetty.getCoreContainer().getCores().iterator().next();
+    CoreContainer coreContainer = jetty.getCoreContainer();
 
-    for (int i = 0; i < (TEST_NIGHTLY ? 5 : 2); i++) {
-      cluster.expireZkSession(jetty);
-      waitForState(
-          "Expecting node to be disconnected", collectionName, activeReplicaCount(1, 0, 0));
-      waitForState("Expecting node to reconnect", collectionName, activeReplicaCount(1, 0, 1));
-      // We have two active ReplicationHandler with two close hooks each, one for triggering
-      // recovery and one for doing interval polling
-      assertEquals(5, core.getCloseHooks().size());
+    // held open across the reconnects below, so the same core instance is checked each time
+    try (SolrCore core = coreContainer.getCore(coreContainer.getLoadedCoreNames().get(0))) {
+      for (int i = 0; i < (TEST_NIGHTLY ? 5 : 2); i++) {
+        cluster.expireZkSession(jetty);
+        waitForState(
+            "Expecting node to be disconnected", collectionName, activeReplicaCount(1, 0, 0));
+        waitForState("Expecting node to reconnect", collectionName, activeReplicaCount(1, 0, 1));
+        // We have two active ReplicationHandler with two close hooks each, one for triggering
+        // recovery and one for doing interval polling
+        assertEquals(5, core.getCloseHooks().size());
+      }
     }
   }
 

solr/core/src/test/org/apache/solr/cloud/TestRandomRequestDistribution.java

@@ -83,21 +83,24 @@ private void testRequestTracking() throws Exception {
 
     ZkStateReader.from(cloudClient).forceUpdateCollection("b1x1");
 
-    // get direct access to the SolrCore objects for each core/replica we're interested to monitor
-    final Map<String, SolrCore> cores = new LinkedHashMap<>();
+    // name + container, not the SolrCore itself: reserved only long enough to read it below
+    final Map<String, CoreContainer> cores = new LinkedHashMap<>();
     for (JettySolrRunner runner : jettys) {
       CoreContainer container = runner.getCoreContainer();
-      for (SolrCore core : container.getCores()) {
-        if ("a1x2".equals(core.getCoreDescriptor().getCollectionName())) {
-          cores.put(core.getName(), core);
+      for (String coreName : container.getLoadedCoreNames()) {
+        try (SolrCore core = container.getCore(coreName)) {
+          if (core == null) continue; // unloaded since getLoadedCoreNames
+          if ("a1x2".equals(core.getCoreDescriptor().getCollectionName())) {
+            cores.put(core.getName(), container);
+          }
         }
       }
     }
     assertEquals("Sanity Check: we know there should be 2 replicas", 2, cores.size());
 
     // Sanity check - all cores should start with 0 requests
-    for (Map.Entry<String, SolrCore> entry : cores.entrySet()) {
-      double initialCount = getSelectRequestCount(entry.getValue());
+    for (Map.Entry<String, CoreContainer> entry : cores.entrySet()) {
+      double initialCount = getSelectRequestCount(entry.getValue(), entry.getKey());
       assertEquals(entry.getKey() + " has already received some requests?", 0L, initialCount, 0.0);
     }
 
@@ -123,8 +126,8 @@ private void testRequestTracking() throws Exception {
       client.query("a1x2", new SolrQuery("*:*"));
 
       double actualTotalRequests = 0;
-      for (Map.Entry<String, SolrCore> entry : cores.entrySet()) {
-        final double coreCount = getSelectRequestCount(entry.getValue());
+      for (Map.Entry<String, CoreContainer> entry : cores.entrySet()) {
+        final double coreCount = getSelectRequestCount(entry.getValue(), entry.getKey());
         actualTotalRequests += coreCount;
         if (0 < coreCount) {
           uniqueCoreNames.add(entry.getKey());
@@ -225,17 +228,16 @@ private void testQueryAgainstDownReplica() throws Exception {
             .withIdleTimeout(5000, TimeUnit.MILLISECONDS)
             .build()) {
 
-      SolrCore leaderCore = null;
+      String leaderCoreName = leader.getStr(ZkStateReader.CORE_NAME_PROP);
+      CoreContainer leaderContainer = null;
       for (JettySolrRunner jetty : jettys) {
         CoreContainer container = jetty.getCoreContainer();
-        for (SolrCore core : container.getCores()) {
-          if (core.getName().equals(leader.getStr(ZkStateReader.CORE_NAME_PROP))) {
-            leaderCore = core;
-            break;
-          }
+        if (container.getLoadedCoreNames().contains(leaderCoreName)) {
+          leaderContainer = container;
+          break;
         }
       }
-      assertNotNull(leaderCore);
+      assertNotNull(leaderContainer);
 
       // All queries should be served by the active replica to make sure that's true we keep
       // querying the down replica. If queries are getting processed by the down replica then the
@@ -246,7 +248,7 @@ private void testQueryAgainstDownReplica() throws Exception {
         count++;
         client.query(new SolrQuery("*:*"));
 
-        double c = getSelectRequestCount(leaderCore);
+        double c = getSelectRequestCount(leaderContainer, leaderCoreName);
 
         if (c == 1) {
           break; // cluster state has got update locally
@@ -267,13 +269,21 @@ private void testQueryAgainstDownReplica() throws Exception {
         client.query(new SolrQuery("*:*"));
         count++;
 
-        double c = getSelectRequestCount(leaderCore);
+        double c = getSelectRequestCount(leaderContainer, leaderCoreName);
 
         assertEquals("Query wasn't served by leader", count, (long) c);
       }
     }
   }
 
+  /** Reserves the named core just long enough to read its /select request count. */
+  private double getSelectRequestCount(CoreContainer container, String coreName) {
+    try (SolrCore core = container.getCore(coreName)) {
+      assertNotNull("Core " + coreName + " is no longer loaded", core);
+      return getSelectRequestCount(core);
+    }
+  }
+
   private Double getSelectRequestCount(SolrCore core) {
     var labels =
         SolrMetricTestUtils.newCloudLabelsBuilder(core)