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.

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

+            .getClusterState()
+            .getCollection(collectionName)
+            .getReplicaStream()
+            .skip(1)


We know we want to skip 1?

Yes -- .skip(1).findFirst() is the exact Stream equivalent of the old .get(1) (0-indexed, second element). The assumption that the added replica lands at index 1 isn't new here, it's inherited from the original .get(1) -- unchanged by this migration.

AI-assisted (Claude Sonnet 5)

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

-            new ReplicaData(replica.getShard(), coreName, (Long) version, numDocs);
-        log.info("{}", data);
-        results.put(coreName, data);
+    for (Slice slice : collectionState) {


I wonder if using a more sonsistent variable name like collectionState instead of colls or c that I saw in other tests would have cued me better on this use of a for loop with Slice?

Done as a separate commit -- renamed the 4 outlier coll/c/colState names to collectionState across the sites this PR touches. Also noticed the CollectionTooManyReplicasTest one you commented on had no extra logic beyond the check, so collapsed it to getReplicaStream().allMatch(...) instead, matching what the majority of sites already do.

AI-assisted (Claude Sonnet 5)

…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.

dsmiley · 2026-08-20T12:50:25Z

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


Eric, if you want to see how Streams can sometimes "do more" in terms of reducing lines of code... well this thing right here screams for Streams. @serhiy-bzhezytskyy hint hint ;-)

Done -- collapsed to getReplicaStream().filter(...).findFirst().orElse(null).

AI-assisted (Claude Sonnet 5)

dsmiley · 2026-08-20T12:50:55Z

-          for (Replica r : c.getReplicas()) {
-            if (r.getState() != Replica.State.ACTIVE) return false;
-          }
-          return true;


could be a Stream one-liner

Already done -- see the earlier reply above, collapsed to getReplicaStream().allMatch(...) a few minutes before this comment was posted.

AI-assisted (Claude Sonnet 5)

dsmiley · 2026-08-20T12:52:27Z

+          for (Slice slice : collectionState) {
+            for (Replica r : slice.getReplicas()) {
+              if (r.isActive(n) == false) return false;
+            }
          }
          return true;


Could be a Stream one-liner.

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

AI-assisted (Claude Sonnet 5)

dsmiley · 2026-08-20T12:54:28Z

    Collections.shuffle(replicas, random());
    return replicas.get(0);


I find this painful. out of scope but trivial to change if you're in the moood

Done -- and made it more elegant per your own "no wasted List accumulation" point: getReplicaStream().collect(Collectors.toCollection(ArrayList::new)) builds the mutable list directly in one pass instead of toList() + a copying new ArrayList<>().

AI-assisted (Claude Sonnet 5)

The painful aspect was hardly the list. It was randomizing the entire list and grabbing the first element. Why do that when we can pick a replica from the list randomly?

dsmiley · 2026-08-20T12:55:03Z

+    for (Slice slice : clusterState.getCollection(DEFAULT_COLLECTION)) {
+      for (Replica replica : slice.getReplicas()) {
+        clients.add(getHttpSolrClient(replica));
+      }


Stream forEach would be shorter

Done -- getReplicaStream().forEach(...).

AI-assisted (Claude Sonnet 5)

dsmiley · 2026-08-20T12:56:58Z

+                              .map(
+                                  c ->
+                                      c.getSlices().stream()
+                                          .mapToInt(s -> s.getReplicas().size())
+                                          .sum())


this is just the count of replicas. Elsewhere you had a far shorter Stream for this

Done -- getReplicaStream().count().

AI-assisted (Claude Sonnet 5)

dsmiley · 2026-08-20T12:58:02Z

-            if (collectionState.getReplicas().size() == 1) {
-              Replica replica = collectionState.getReplicas().get(0);
+            List<Replica> replicas =
+                collectionState.getReplicaStream().collect(Collectors.toList());


Tell you're AI we're on Java 21 here and thus can call Stream.toList()

Done -- Stream.toList() at both sites.

AI-assisted (Claude Sonnet 5)

dsmiley · 2026-08-20T12:58:20Z

-            if (collectionState.getReplicas().size() == 1) {
-              Replica replica = collectionState.getReplicas().get(0);
+            List<Replica> replicas =
+                collectionState.getReplicaStream().collect(Collectors.toList());


Same fix.

AI-assisted (Claude Sonnet 5)

dsmiley · 2026-08-20T12:59:04Z

+    for (Slice slice : clusterState.getCollection(COLL)) {
+      for (Replica replica : slice.getReplicas()) {
+        clients.add(
+            new CollectionScopedSolrClient(
+                cluster.getReplicaJetty(replica).getSolrClient(), replica.getCoreName()));
+      }


consider Stream to avoid double-loop

Done -- same fix.

AI-assisted (Claude Sonnet 5)

"...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.

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

-    }
-    return replicas;
+  /** Stream of all replicas across all slices. */
+  public Stream<Replica> getReplicaStream() {


Since we don't merely get something we have (we don't already have a Stream), maybe the name of this method should be replicaStream ? OTOH, nobody ought to ever think a Stream returning method is saved/persisted; they are always ephemeral. Hoping to here from @serhiy-bzhezytskyy and not your AI.

:) i think it is obvious that it smells now ...
look at this one: getReplicaStream().filter(...).findFirst().orElse(null).
it doesn't look fluently (aka Fluent API), right? so i think it should be replicaStream().filter(...).findFirst().orElse(null). etc
@dsmiley if it does make sense for you/us then i will be happy (not AI term) to make such refactoring not only here but we i will feel the same smell

Cool let's do the rename. To be clear, I meant I wanted your opinion, not an AI's opinion specifically here.

…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.

dsmiley · 2026-08-21T02:12:03Z

@@ -0,0 +1,8 @@
+# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
+title: Remove the deprecated DocCollection.getReplicas() method, which allocated a new list on every call. DocCollection is Iterable<Slice>, so iterate its slices and call Slice.getReplicas(), or use getSlices().stream().flatMap(slice -> slice.getReplicas().stream()).


Let's be brief on internal matters where even a changelog is debatable.

Suggested change

title: Remove the deprecated DocCollection.getReplicas() method, which allocated a new list on every call. DocCollection is Iterable<Slice>, so iterate its slices and call Slice.getReplicas(), or use getSlices().stream().flatMap(slice -> slice.getReplicas().stream()).

title: Remove DocCollection.getReplicas() and add similar replicaStream()

As I read this... gosh, we're wasting time with such a changelog. That's the thing -- ask yourself, will a user of Solr care? Honestly, no.

dsmiley · 2026-08-21T02:13:52Z

-        .map(Replica::getNodeName)
-        .distinct()
-        .collect(Collectors.toList());
+    return state.replicaStream().map(Replica::getNodeName).distinct().collect(Collectors.toList());


Suggested change

return state.replicaStream().map(Replica::getNodeName).distinct().collect(Collectors.toList());

return state.replicaStream().map(Replica::getNodeName).distinct().toList();

…ate var

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

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

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

d1d242c

…ate var

		Collections.shuffle(replicas, random());
		return replicas.get(0);

		@@ -0,0 +1,8 @@
		# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
		title: Remove the deprecated DocCollection.getReplicas() method, which allocated a new list on every call. DocCollection is Iterable<Slice>, so iterate its slices and call Slice.getReplicas(), or use getSlices().stream().flatMap(slice -> slice.getReplicas().stream()).

	title: Remove the deprecated DocCollection.getReplicas() method, which allocated a new list on every call. DocCollection is Iterable<Slice>, so iterate its slices and call Slice.getReplicas(), or use getSlices().stream().flatMap(slice -> slice.getReplicas().stream()).
	title: Remove DocCollection.getReplicas() and add similar replicaStream()

	return state.replicaStream().map(Replica::getNodeName).distinct().collect(Collectors.toList());
	return state.replicaStream().map(Replica::getNodeName).distinct().toList();

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!

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!

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

serhiy-bzhezytskyy commented Aug 19, 2026 •

edited

Loading