SOLR-18378: remove CoreContainer.getCores() and SolrCores.getCores(), 26 sites by serhiy-bzhezytskyy · Pull Request #4764 · apache/solr

serhiy-bzhezytskyy · 2026-08-19T17:08:55Z

https://issues.apache.org/jira/browse/SOLR-18378

Removes CoreContainer.getCores()/SolrCores.getCores() — 26 sites. Not a rename: the removed methods handed out unreserved cores; the replacement (getLoadedCoreNames() + getCore(String)) reserves, so every site now acquires and releases.

Where to look: two shutdown paths (cancelCoreRecoveries, pauseUpdatesAndAwaitInflightRequests) can't use getCore — it can load a core, wrong during shutdown — so they use getCoreFromAnyList(name, true) instead, which never loads. TestTlogReplica's two helpers deliberately return already-released cores, matching the old contract, to avoid an 11-site refactor out of scope here.

Also fixed a dangling {@link #getCores()} — only ecjLintMain catches that, not the compiler or renderJavadoc.

93 tests / 15 classes / 0 failures. Simulated merge order against SOLR-18382 (#4760) and SOLR-18380 (#4762), which also touch shared files — applies cleanly either way.

AI-assisted (Claude Sonnet 5)

… 26 sites Not a rename. The removed methods handed out cores that were NOT reference counted - their own javadoc said both "rather dangerous API because each core is not reserved" and "Don't need to close them". The replacement, getLoadedCoreNames() plus getCore(String), hands out a reserved core that the caller must close. So every migrated site now acquires a refcount and every site has to release it, and a mechanical replace here would leak one. A leaked refcount does not fail a test; it hangs a shutdown. The documented replacement is also subtly wrong in two places, and reading it was the only way to find out. getCore(name) is not a pure lookup - its own comment says "Do this in two phases since we don't want to lock access to the cores over a load", and in standalone mode it can create a core from its descriptor. The removed method returned only already-loaded cores. Both CoreContainer.cancelCoreRecoveries and pauseUpdatesAndAwaitInflightRequests are shutdown paths, where loading a core would be actively wrong, so those two use solrCores.getCoreFromAnyList(name, true), which reserves an already-loaded core and never loads. Three treatments, chosen per site rather than applied uniformly: - pure count (3 sites): getLoadedCoreNames().size() - reserves nothing at all - iterate all cores (5 sites): for (name : getLoadedCoreNames()) try (core = getCore(name)), with a null guard because the core may be unloaded between the two calls - the reference escapes the expression (8 sites): restructured so the use sits inside the try. Where a site only needed the core's name, it now takes the name and opens no core. getLoadedCoreNames() is unsorted exactly as getCores() was, so sites picking .get(0) or iterator().next() keep the same arbitrary choice. Nothing was sorted. One deliberate exception, stated in the code. TestTlogReplica's two private helpers return SolrCore references consumed at 11 call sites combined, one of which uses the cores to stop the jettys hosting them; propagating reservation there is exactly the hang hazard above. Those helpers reserve-then-release inside their own try, returning already- closed instances - the removed API's own contract - with a comment saying so. The alternative is an 11-site refactor of a test, which is not this ticket. The census took four compile rounds - 5 production sites, then 3 in test-framework/src/java which ships, then 16 in tests, then 2 more in modules/ltr - because each round's failures stopped the next module from compiling at all. 26 total. Also fixed: a dangling {@link #getCores()} on SolrCores.getNumLoadedPermanentCores, and a commented-out sketch in TimeAllowedTest that named the removed method. Measured which gate catches the dangling link, because it is not the obvious one: compileJava passes, renderJavadoc passes, and ecjLintMain is what fails. Verified: compileJava and compileTestJava for the whole build with zero errors, spotlessCheck, ecjLintMain and ecjLintTest on core, ecjLintMain on test-framework and ltr, renderJavadoc, and 15 changed test classes (counting TestTlogReplica's helper subclasses via their own suites) - 93 tests, 0 failures. Simulated merge order against SOLR-18382 (open PR, shares test-framework cloud base classes and TestTlogReplica) and SOLR-18380 (open PR, shares the same test-framework files) - applying this diff on top of either merged first, and both merged together, succeeds cleanly. AI-assisted (Claude Sonnet 5)

serhiy-bzhezytskyy · 2026-08-19T19:10:38Z

@dsmiley you deprecated getCores() on both CoreContainer and SolrCores back in 2023. Worth flagging directly: the javadoc's own suggested replacement (getCore(name)) turns out to load a core if it isn't already loaded -- wrong on 2 shutdown paths, where that's exactly what must not happen. Used getCoreFromAnyList(name, true) instead. Given it's your deprecation and the fix touches a subtlety in the reference-counting contract, a review here would carry real weight.

AI-assisted (Claude Sonnet 5)

dsmiley

Thanks. I wonder if we should have a CoreContainer.forEachCore(Consumer<SolrCore>))

dsmiley · 2026-08-20T00:31:04Z

+      // getCoreFromAnyList, not getCore: never loads, safe during shutdown
+      try (SolrCore core = solrCores.getCoreFromAnyList(coreName, true)) {


how could this be correct, using try-with-resources on a getCore that has not been inc-ref'ed, and thus we shouldn't close it.

getCoreFromAnyList(name, true) does inc-ref -- the second param is literally named incRefCount, and its body calls core.open(), whose own javadoc says "expert: increments the core reference count". So try-with-resources's close() here releases exactly that reference, not an un-reserved one. Same pattern is already used elsewhere in this file (SolrCore.java:3447/3482).

AI-assisted (Claude Sonnet 5)

Went further and wrote a concurrency stress test for this exact concern (production, not just the single-threaded mechanism) -- 3 threads acquiring/releasing via getCoreFromAnyList(name, true) while a 4th concurrently unloads/reloads the same core. 0 failures across ~730k acquisitions in 4 runs. Added as TestCoreContainer.testGetCoreFromAnyListSafeUnderConcurrentUnload, pushed.

AI-assisted (Claude Sonnet 5)

Okay I misunderstood your other PR comments where I thought you/AI communicated solrCores.getCoreFromAnyList doesn't incref which is why you were using it. Now I actually looked at our code to confirm.

dsmiley · 2026-08-20T00:31:47Z


  public void cancelCoreRecoveries() {
-
-    List<SolrCore> cores = solrCores.getCores();


Arguably, SolrCores.getCores() shouldn't be deprecated because it's hidden one layer deep on a class only used by CoreContainer.

Fair -- checked, and SolrCores is package-private and only ever referenced from CoreContainer, so the deprecation was arguably unnecessary ceremony from the start. Not making a case for the original deprecation though -- the ticket scope was to remove both getCores() methods together, so that's what this PR does.

AI-assisted (Claude Sonnet 5)

dsmiley · 2026-08-20T00:32:28Z

-   *     _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


maybe shouldn't be deprecated after all (as I say above

Same as the CoreContainer.java:1386 thread -- fair point, no argument.

AI-assisted (Claude Sonnet 5)

I think either we bring this back, or we add the method I suggested to CoreContainer forEachLoadedCore(Consumer<SolrCore>))

Added CoreContainer.forEachLoadedCore(Consumer<SolrCore>), migrated cancelCoreRecoveries() to it.

dsmiley · 2026-08-20T00:33:19Z

nice; this is safer as we avoid race condition on a core closing with inspecting its health

dsmiley · 2026-08-20T00:33:49Z

again; added safety :-)

David Smiley's review claimed the core in cancelCoreRecoveries() has "not been inc-ref'ed", so try-with-resources closing it would be wrong. Verified false: incRefCount=true calls core.open(), same pattern already used at SolrCore.java:3447/3482. Proven live on a single-threaded probe (openCount 1 -> 2 -> 1 across acquire/close). This test extends that to the actual concern raised -- production concurrency: 3 threads repeatedly acquire/use/release via getCoreFromAnyList(name, true) while a 4th thread concurrently unloads/reloads the same core name. No existing test exercises this; the one similar-looking loop (SimpleCollectionCreateDeleteTest.java, also touched by this PR) uses the unrelated CoreContainer.getCore(name), not SolrCores.getCoreFromAnyList. Ran 4x before formatting, ~730k total acquisitions, 0 failures. AI-assisted (Claude Sonnet 5)

epugh · 2026-08-20T10:07:24Z

Just retriggered the CI processes. There is enough change here with ramifications, I'll wait for approved review from someone like @dsmiley before merging.

dsmiley · 2026-08-20T13:14:24Z

+      // getCoreFromAnyList, not getCore: never loads, safe during shutdown
+      try (SolrCore core = solrCores.getCoreFromAnyList(coreName, true)) {


Okay I misunderstood your other PR comments where I thought you/AI communicated solrCores.getCoreFromAnyList doesn't incref which is why you were using it. Now I actually looked at our code to confirm.

dsmiley · 2026-08-20T13:19:47Z

-   *     _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


I think either we bring this back, or we add the method I suggested to CoreContainer forEachLoadedCore(Consumer<SolrCore>))

dsmiley · 2026-08-20T13:21:18Z

very thorough of your LLM to do this on the account of my inquiry but we can lose this

dsmiley · 2026-08-20T13:50:43Z

The changelog is debatable but I think the more important (value producing aspect) here is that this reduced risks of SolrCore closing while being used. The methods themselves are internal stuff a reader shouldn't care about.

…ist" This reverts commit 7b012ef.

David's suggestion on SolrCores.java:152 -- migrates cancelCoreRecoveries() to it, the only site requested in review. NodeHealth/SolrPackageLoader keep their current getCore()-based loops unchanged (different semantics: getCore() can lazy-reload on a race, forEachLoadedCore deliberately never does).

github-actions Bot added test-framework module:ltr tests cat:cloud cat:index cat:packagemanager labels Aug 19, 2026

dsmiley self-requested a review August 20, 2026 00:26

dsmiley reviewed Aug 20, 2026

View reviewed changes

serhiy-bzhezytskyy added 2 commits August 20, 2026 18:20

Revert "SOLR-18378: add a concurrency stress test for getCoreFromAnyL…

b0f6cd7

…ist" This reverts commit 7b012ef.

		// getCoreFromAnyList, not getCore: never loads, safe during shutdown
		try (SolrCore core = solrCores.getCoreFromAnyList(coreName, true)) {


		public void cancelCoreRecoveries() {

		List<SolrCore> cores = solrCores.getCores();

Uh oh!

Conversation

serhiy-bzhezytskyy commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-bzhezytskyy commented Aug 19, 2026

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epugh commented Aug 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsmiley commented Aug 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

serhiy-bzhezytskyy commented Aug 19, 2026 •

edited

Loading