SOLR-18382: remove DocCollection.getReplicas(), migrate 60 call sites by serhiy-bzhezytskyy · Pull Request #4760 · apache/solr

serhiy-bzhezytskyy · 2026-08-19T14:47:25Z

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

Removes DocCollection.getReplicas() (flattened every slice into a fresh ArrayList on each call) and migrates 60 call sites — the real count, not the 285 a plain .getReplicas() grep returns, since Slice.getReplicas() shares the name. The method was deleted first and the compiler's error list became the worklist.

Split by source set: 2 production sites, 3 in test-framework (a published artifact), 55 across 32 test files — that's why this ticket is large despite "low usage" being correct about production.

Where to look: CollectionTooManyReplicasTest.java has a local named slice already in scope at one call site, so the migrated loop there uses s instead — everywhere else uses slice. Three checks before trusting the swap: traversal order is unchanged (nothing was sorted), the three .toList() sites are read-only, and every asserted literal survives verbatim.

413 tests across 32 changed classes, 0 failures. Compile is the actual census here — a failing build's first report showed 2 sites; the true number surfaced only once test-framework compiled too.

SOLR-18378, SOLR-18380, SOLR-18381 and SOLR-18385 touch files this PR also touches — merging this one first should make those cleaner to extract.

AI-assisted (Claude Sonnet 5)

The method flattened every slice's replicas into a fresh ArrayList on each call. DocCollection is already Iterable<Slice>, so callers iterate slices, or use the idiom the tree already had in four places: X.getSlices().stream().flatMap(slice -> slice.getReplicas().stream()) The census had to come from the compiler, not from grep. `git grep '\.getReplicas()'` returns 285 hits and only 60 are this method: the name is declared seven times in the tree, and Slice.getReplicas() accounts for most of the rest. No textual pattern can separate them, because the receiver's name carries no type - the 60 real sites are reached through eleven different variable names including `slices`, which is a DocCollection named as if it were a Slice. So the method was deleted first and the error list became the worklist. And a failing build is not a complete census. The first compile reported 2 call sites; the true number was 60. It had died in test-framework, so core's test compilation never ran. The list is the fixed point of delete-compile-fix, not the first report. Split by source set, because the two halves answer different questions: production src/java 2 sites - the whole compatibility surface test-framework/src/java 3 sites - a published artifact, so this counts as API src/test 55 sites in 32 files That makes the deprecation note - "low usage and builds an ArrayList (surprising)" - correct about what it cares about. Two production callers is low usage. It is the test migration that makes the ticket large. Three things the type change could have broken, all checked. The removed method returned List<Replica> while Slice.getReplicas() returns Collection<Replica>, so sites needing indexed access collect to a real list; the three .toList() sites are read-only, including the one passed to assertDocsExistInAllReplicas, whose two overloads only iterate their argument. Traversal order is unchanged, so sites doing .get(0) or iterator().next() still pick the same replica; nothing was sorted. No asserted value was altered - every expected literal (1, 2, 4, 8, 8, 9) and every assertion message survives verbatim, and of 147 removed lines, 77 reappear identically modulo indentation from the added nesting. One conversion needed care: turning a single loop into a nested pair inside a lambda collided with an enclosing local named `slice` in CollectionTooManyReplicasTest, which Java forbids; the loop variable is `s` there. Verified: compileJava and compileTestJava for the whole build, spotlessCheck, ecjLintMain on core, solrj and test-framework, and all 32 changed test classes - 413 tests, 0 failures, 70 skipped, with every class confirmed to have produced a result file rather than being silently filtered out. AI-assisted (Claude Sonnet 5)

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

@dsmiley this one's yours (DocCollection.getReplicas(), deprecated Dec 2024) and it's the biggest migration in the batch -- 60 call sites across 37 files. Given the scale, a look from you before this merges would be genuinely useful, not just a courtesy tag.

AI-assisted (Claude Sonnet 5)

dsmiley

Thanks.
Admittedly it adds more code in many places. Maybe a getReplicaStream() method would be useful?

David Smiley's review on this PR: "Admittedly it adds more code in many places. Maybe a getReplicaStream() method would be useful?" It would -- 36 sites use the identical getSlices().stream().flatMap(slice -> slice.getReplicas().stream()) two-liner, 8 more use .mapToInt(s -> s.getReplicas().size()).sum() for a count. Both collapse to one call each. Left the ~12 manual for-loops alone, some carry extra logic inline. AI-assisted (Claude Sonnet 5)

serhiy-bzhezytskyy · 2026-08-20T06:08:24Z

Thanks! Added getReplicaStream() and refactored the 44 call sites that would benefit -- 36 identical .getSlices().stream().flatMap(slice -> slice.getReplicas().stream()) blocks and 8 .mapToInt(s -> s.getReplicas().size()).sum() counts collapse to one call each. Left the dozen manual for-loops alone since some carry extra logic inline.

25 test classes re-verified, including the two @nightly ones this touches (ShardSplitTest, TestPullReplica) -- 0 failures.

P.S. Three more sites use the same pattern outside this PR's diff -- CollectionHandlingUtils.java and CreateCollectionCmd.java (production code, not tests) plus CreateRoutedAliasTest.java -- left alone here since they're out of scope, but getReplicaStream() would help there too.

AI-assisted (Claude Sonnet 5)

epugh

Bit more on hte fence on this one... I thought this would make the code easier to read, and i think it's harder... not sure the getReplicaStream is very useful... Especially when we add more .findFirst or .orThrows type clauses...

epugh · 2026-08-20T10:10:41Z

-    for (Replica r : coll.getReplicas()) {
-      if (replicaName.equals(r.getCoreName())) {
-        return r;
+    for (Slice slice : coll) {


i am not quite getting this line? is coll a array and we are iterating over it? Or is col a single value, and we just map it to slice, so this for loop only fires actually 1 time?

coll is a DocCollection, which is Iterable<Slice> -- a collection has one or more shards, so the outer loop runs once per shard, not once total. This is a direct unroll of what the old getReplicas() did internally (for (Slice slice : this) { replicas.addAll(slice.getReplicas()); }), just inlined instead of calling the now-removed method.

AI-assisted (Claude Sonnet 5)

epugh · 2026-08-20T10:11:15Z

        c -> {
-          for (Replica r : c.getReplicas()) {
-            if (r.getState() != Replica.State.ACTIVE) return false;
+          for (Slice s : c) {


same not sure if we need this for loop?

Same as the ReindexCollectionCmd.java:718 thread -- c iterates its shards, one loop per shard.

AI-assisted (Claude Sonnet 5)

Follow-up: simplified this one further to docCollection.getReplicaStream().allMatch(r -> r.getState() == Replica.State.ACTIVE) -- no extra logic here beyond the check, so it collapses cleanly.

AI-assisted (Claude Sonnet 5)

epugh · 2026-08-20T10:13:13Z

            .getClusterState()
            .getCollection(AbstractFullDistribZkTestBase.DEFAULT_COLLECTION);
-    Replica replica = defCol.getReplicas().get(0);
+    Replica replica = defCol.getReplicaStream().findFirst().orElseThrow();


I suppose this is fine, but seems like there is some complex logic that maybe was hidden by teh get(0) that you now need to remember.. I think reading this that get(0) threw a exepction maybe if there are no replicas, but now with findFirst you have to add the .orElseThrow() to get the same behavior? I don't know if that is something that will trip people up? Or maybe the whole get(0) throwing an exception wasn't great to start with. Just a comment.

Checked both messages directly -- orElseThrow() throws NoSuchElementException: No value present, get(0) threw IndexOutOfBoundsException: Index 0 out of bounds for length 0. Different exception type, but neither is less clear than the other, so I don't think this trips anyone up. All 7 migrated getReplicaStream().findFirst() sites use .orElseThrow() the same way, none swallows an empty stream.

AI-assisted (Claude Sonnet 5)

IMO this is beautiful/elegant and clear. @epugh , I take it you are not familiar (or is a hater of) the Java Stream api.

epugh · 2026-08-20T10:14:08Z

    assertNotNull(docCollection);
    // sanity check that everything is as before
-    assertEquals(9, docCollection.getReplicas().size());
+    assertEquals(9, docCollection.getReplicaStream().count());


I am not sure I see a big benefit in the getReplicaStream over getReplica.stream..

I think I was excpecting getReplicaStream to do more!

Fair -- for this specific line there's no real win, you're right, .getReplicaStream().count() isn't fancier than .getReplicas().size() was. The method's main job is replacing the verbose getSlices().stream().flatMap(slice -> slice.getReplicas().stream()) two-liner (73% of migrated call sites had that shape) -- this site just needed some replacement for the removed getReplicas(), and this is the plain flatten, not meant to do more.

AI-assisted (Claude Sonnet 5)

Streams sometimes "does more" behind the scenes, and that happened here. No internal wasted List accumulation since we don't need a list to count.

…or consistency Eric Pugh noted a clearer, consistent name would have cued the Slice loop's meaning better. Renamed the outlier `coll`/`c`/`colState` names to `collectionState` at 4 sites this PR already touches; also collapsed CollectionTooManyReplicasTest's plain "all replicas active" loop (no extra logic beyond the check) to getReplicaStream().allMatch(), matching the rule the other sites already followed.

"...without allocating an intermediate list" named the removed method's behavior by negation instead of just stating the fact.

…one-liner Pure find-first-match with no extra logic, same shape as the other migrated sites -- collapses to filter().findFirst().orElse(null).

Short-circuit && preserves the early return-false on shard1 not being active, then getReplicaStream().allMatch() replaces the manual double loop.

Still needs a mutable List for Collections.shuffle(), so wrapped in a new ArrayList<> rather than returning the stream's own immutable list.

…e allocation toList() + new ArrayList<>(...) built two lists; Collectors.toCollection builds the mutable one directly in a single pass.

…view Pure per-replica assertion with no accumulation -- getReplicaStream() + forEach() replaces the double loop.

…tion

Solr is on Java 21; both sites only read the list (size()/get(0)), so the immutable list from toList() is a safe drop-in.

David: we aren't returning something we already have, so a getter-style prefix is wrong for a Stream-returning method -- matches the JDK's own convention (Collection.stream(), not Collection.getStream()).

David: we don't need to manipulate the list merely to pick one at random -- toList() (immutable) + a random index avoids the shuffle and the mutable-list requirement entirely.

…ate var

dsmiley's call: 'Let's be brief on internal matters where even a changelog is debatable... will a user of Solr care? Honestly, no.' Same standard he applied on SOLR-18374/SOLR-18353.

# Conflicts: # solr/core/src/java/org/apache/solr/handler/designer/SchemaDesignerConfigSetHelper.java

…eplicas() call solr:webapp wasn't covered by this PR's own migration sweep -- same recurring gap as SOLR-18390/apache#4778 and SOLR-18357/apache#4790. replicaCount() still called the now-removed getReplicas().size(); migrated to replicaStream().count() (cast to int, matching the method's return type), the same pattern used at every other call site in this PR.

serhiy-bzhezytskyy · 2026-08-24T04:39:40Z

@epugh could you add the no-changelog label here?

…collection-getreplicas

github-actions Bot added test-framework jetty-server client:solrj tests cat:search cat:cloud cat:index cat:api labels Aug 19, 2026

This was referenced Aug 19, 2026

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

Open

SOLR-18385: remove CollectionAdminRequest.PROPERTY_PREFIX #4770

Merged

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

dsmiley approved these changes Aug 20, 2026

View reviewed changes

dsmiley added this to the 10.x milestone Aug 20, 2026

epugh reviewed Aug 20, 2026

View reviewed changes

dsmiley reviewed Aug 20, 2026

View reviewed changes

serhiy-bzhezytskyy added 4 commits August 20, 2026 16:19

SOLR-18382: rewrite a javadoc line that described what it did NOT do

803ed76

"...without allocating an intermediate list" named the removed method's behavior by negation instead of just stating the fact.

SOLR-18382: collapse getReplicaForDaemon's double-loop into a Stream …

a133f29

…one-liner Pure find-first-match with no extra logic, same shape as the other migrated sites -- collapses to filter().findFirst().orElse(null).

SOLR-18382: collapse another predicate to a Stream one-liner

02818cb

Short-circuit && preserves the early return-false on shard1 not being active, then getReplicaStream().allMatch() replaces the manual double loop.

SOLR-18382: replace manual accumulation with getReplicaStream().toList()

0b39c99

Still needs a mutable List for Collections.shuffle(), so wrapped in a new ArrayList<> rather than returning the stream's own immutable list.

dsmiley reviewed Aug 20, 2026

View reviewed changes

Comment thread solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java Outdated

serhiy-bzhezytskyy added 5 commits August 20, 2026 16:51

SOLR-18382: collect directly into a mutable ArrayList, skip the doubl…

5c2fc0c

…e allocation toList() + new ArrayList<>(...) built two lists; Collectors.toCollection builds the mutable one directly in a single pass.

SOLR-18382: collapse another loop found via census, not flagged by re…

ffb38a3

…view Pure per-replica assertion with no accumulation -- getReplicaStream() + forEach() replaces the double loop.

SOLR-18382: replace the double-loop with forEach, per David's suggestion

a71a0ef

SOLR-18382: replace the double-loop with a Stream, per David's sugges…

827b9d7

…tion

SOLR-18382: collapse the find-first-then-check predicate to a Stream

ed22c64

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

SOLR-18382: use getReplicaStream().count() for the total-replica count

293d6ca

SOLR-18382: use Stream.toList() instead of collect(Collectors.toList())

afa2b44

Solr is on Java 21; both sites only read the list (size()/get(0)), so the immutable list from toList() is a safe drop-in.

dsmiley reviewed Aug 20, 2026

View reviewed changes

Comment thread solr/core/src/test/org/apache/solr/cloud/MoveReplicaTest.java Outdated

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

SOLR-18382: rename getReplicaStream() to replicaStream()

64a7341

David: we aren't returning something we already have, so a getter-style prefix is wrong for a Stream-returning method -- matches the JDK's own convention (Collection.stream(), not Collection.getStream()).

SOLR-18382: pick a random replica by index instead of shuffling

aee88ea

David: we don't need to manipulate the list merely to pick one at random -- toList() (immutable) + a random index avoids the shuffle and the mutable-list requirement entirely.

dsmiley reviewed Aug 21, 2026

View reviewed changes

Comment thread changelog/unreleased/SOLR-18382-remove-doccollection-getreplicas.yml Outdated

Comment thread solr/core/src/test/org/apache/solr/cloud/api/collections/CollectionTooManyReplicasTest.java Outdated

serhiy-bzhezytskyy added 4 commits August 21, 2026 07:47

SOLR-18382: use toList() in getAllNodeNames, inline the single-use st…

d1d242c

…ate var

SOLR-18382: drop the changelog entry

1c8e77c

dsmiley's call: 'Let's be brief on internal matters where even a changelog is debatable... will a user of Solr care? Honestly, no.' Same standard he applied on SOLR-18374/SOLR-18353.

Merge branch 'main' into SOLR-18382-remove-doccollection-getreplicas

bedc5dd

# Conflicts: # solr/core/src/java/org/apache/solr/handler/designer/SchemaDesignerConfigSetHelper.java

github-actions Bot added the admin-ui label Aug 23, 2026

Merge remote-tracking branch 'origin/main' into SOLR-18382-remove-doc…

bd49be2

…collection-getreplicas

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!

serhiy-bzhezytskyy commented Aug 20, 2026

Uh oh!

epugh 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

serhiy-bzhezytskyy commented Aug 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

serhiy-bzhezytskyy commented Aug 19, 2026 •

edited

Loading