Uh oh!
There was an error while loading. Please reload this page.
SOLR-18378: remove CoreContainer.getCores() and SolrCores.getCores(), 26 sites - #4764
Conversation
… 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
commented
Aug 19, 2026
@dsmiley you deprecated AI-assisted (Claude Sonnet 5) |
dsmiley
left a comment
There was a problem hiding this comment.
Thanks. I wonder if we should have a CoreContainer.forEachCore(Consumer<SolrCore>))
| // getCoreFromAnyList, not getCore: never loads, safe during shutdown | ||
| try (SolrCore core = solrCores.getCoreFromAnyList(coreName, true)) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
| public void cancelCoreRecoveries() { | ||
| List<SolrCore> cores = solrCores.getCores(); |
There was a problem hiding this comment.
Arguably, SolrCores.getCores() shouldn't be deprecated because it's hidden one layer deep on a class only used by CoreContainer.
There was a problem hiding this comment.
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)
| * _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 |
There was a problem hiding this comment.
maybe shouldn't be deprecated after all (as I say above
There was a problem hiding this comment.
Same as the CoreContainer.java:1386 thread -- fair point, no argument.
AI-assisted (Claude Sonnet 5)
There was a problem hiding this comment.
I think either we bring this back, or we add the method I suggested to CoreContainer forEachLoadedCore(Consumer<SolrCore>))
There was a problem hiding this comment.
Added CoreContainer.forEachLoadedCore(Consumer<SolrCore>), migrated cancelCoreRecoveries() to it.
There was a problem hiding this comment.
nice; this is safer as we avoid race condition on a core closing with inspecting its health
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
commented
Aug 20, 2026
Just retriggered the CI processes. There is enough change here with ramifications, I'll wait for approved review from someone like @dsmiley before merging. |
| // getCoreFromAnyList, not getCore: never loads, safe during shutdown | ||
| try (SolrCore core = solrCores.getCoreFromAnyList(coreName, true)) { |
There was a problem hiding this comment.
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.
| * _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 |
There was a problem hiding this comment.
I think either we bring this back, or we add the method I suggested to CoreContainer forEachLoadedCore(Consumer<SolrCore>))
Uh oh!
There was an error while loading. Please reload this page.
dsmiley
commented
Aug 20, 2026
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).
dsmiley
left a comment
There was a problem hiding this comment.
I like the new method but why aren't you calling it nearly everywhere possible?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…EachLoadedCore dsmiley asked on the PR why these two call sites, which iterate getLoadedCoreNames() and reopen each via getCore(coreName), weren't migrated when forEachLoadedCore was added. Turns out they carry the same bug as the one this ticket already fixed elsewhere: getCore(name) LOADS the core if it was unloaded between the getLoadedCoreNames() snapshot and the getCore() call, whereas forEachLoadedCore's getCoreFromAnyList(name, true) never does. SolrPackageLoader: a package-update notification or a health check should never have the side effect of force-loading an unloaded core. NodeHealth's healthCheckStandaloneMode needed a mutable AtomicBoolean instead of a plain local, since it's now captured by the forEachLoadedCore lambda. Verified: NodeHealthTest, NodeHealthStandaloneTest, TestPackages green; full compileJava clean.
serhiy-bzhezytskyy
commented
Aug 22, 2026
Fair — pushed a follow-up commit migrating AI-assisted (Claude Sonnet 5) |
serhiy-bzhezytskyy
commented
Aug 22, 2026
To be fair, the reason |
serhiy-bzhezytskyy
commented
Aug 23, 2026
Removed the stress test. |
And avoid some needless core incref/decref.
…econtainer-getcores
dsmiley
commented
Aug 24, 2026
Yesterday I committed changes to use forEachLoadedCore much more. I anticipate merging today. |
Left dangling by an earlier commit on this branch that replaced its one use site; failed ecjLint + spotlessJavaCheck in CI.
Uh oh!
There was an error while loading. Please reload this page.
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 usegetCore— it can load a core, wrong during shutdown — so they usegetCoreFromAnyList(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()}— onlyecjLintMaincatches that, not the compiler orrenderJavadoc.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)