dsmiley commented on code in PR #4760:
URL: https://github.com/apache/solr/pull/4760#discussion_r3821693420
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/ReindexCollectionCmd.java:
##########
@@ -714,9 +715,11 @@ private Replica getReplicaForDaemon(SolrResponse rsp,
DocCollection coll) {
return null;
}
// build a baseUrl of the replica
- for (Replica r : coll.getReplicas()) {
- if (replicaName.equals(r.getCoreName())) {
- return r;
+ for (Slice slice : collectionState) {
+ for (Replica r : slice.getReplicas()) {
+ if (replicaName.equals(r.getCoreName())) {
+ return r;
+ }
}
}
Review Comment:
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 ;-)
##########
solr/core/src/test/org/apache/solr/cloud/CollectionsAPISolrJTest.java:
##########
@@ -409,10 +409,12 @@ public void testSplitShard() throws Exception {
waitForState(
"Expected all shards to be active and parent shard to be removed",
collectionName,
- (n, c) -> {
- if (c.getSlice("shard1").getState() == Slice.State.ACTIVE) return
false;
- for (Replica r : c.getReplicas()) {
- if (r.isActive(n) == false) return false;
+ (n, collectionState) -> {
+ if (collectionState.getSlice("shard1").getState() ==
Slice.State.ACTIVE) return false;
+ for (Slice slice : collectionState) {
+ for (Replica r : slice.getReplicas()) {
+ if (r.isActive(n) == false) return false;
+ }
}
return true;
Review Comment:
Could be a Stream one-liner.
##########
solr/core/src/test/org/apache/solr/cloud/api/collections/CollectionTooManyReplicasTest.java:
##########
@@ -112,12 +112,8 @@ public void testAddTooManyReplicas() throws Exception {
waitForState(
"Expected to see all replicas active",
collectionName,
- c -> {
- for (Replica r : c.getReplicas()) {
- if (r.getState() != Replica.State.ACTIVE) return false;
- }
- return true;
Review Comment:
could be a Stream one-liner
##########
solr/core/src/test/org/apache/solr/cloud/NestedShardedAtomicUpdateTest.java:
##########
@@ -71,8 +72,10 @@ public static void beforeClass() throws Exception {
clients = new ArrayList<>();
ClusterState clusterState = cloudClient.getClusterState();
- for (Replica replica :
clusterState.getCollection(DEFAULT_COLLECTION).getReplicas()) {
- clients.add(getHttpSolrClient(replica));
+ for (Slice slice : clusterState.getCollection(DEFAULT_COLLECTION)) {
+ for (Replica replica : slice.getReplicas()) {
+ clients.add(getHttpSolrClient(replica));
+ }
Review Comment:
Stream forEach would be shorter
##########
solr/core/src/test/org/apache/solr/cloud/ZkControllerTest.java:
##########
@@ -373,8 +373,11 @@ public List<CoreDescriptor> getCoreDescriptors() {
TimeUnit.SECONDS,
collectionState ->
Optional.ofNullable(collectionState)
- .map(DocCollection::getReplicas)
- .map(List::size)
+ .map(
+ c ->
+ c.getSlices().stream()
+ .mapToInt(s ->
s.getReplicas().size())
+ .sum())
Review Comment:
this is just the count of replicas. Elsewhere you had a far shorter Stream
for this
##########
solr/core/src/test/org/apache/solr/search/TestCoordinatorRole.java:
##########
@@ -798,8 +798,10 @@ public void testMoveReplica() throws Exception {
"Cannot find replica on first node yet",
COLLECTION_NAME,
collectionState -> {
- if (collectionState.getReplicas().size() == 1) {
- Replica replica = collectionState.getReplicas().get(0);
+ List<Replica> replicas =
+
collectionState.getReplicaStream().collect(Collectors.toList());
Review Comment:
Tell you're AI we're on Java 21 here and thus can call Stream.toList()
##########
solr/core/src/test/org/apache/solr/cloud/MoveReplicaTest.java:
##########
@@ -342,7 +342,10 @@ private CollectionAdminRequest.MoveReplica
createMoveReplicaRequest(
}
private Replica getRandomReplica(String coll, CloudSolrClient cloudClient)
throws IOException {
- List<Replica> replicas =
cloudClient.getClusterState().getCollection(coll).getReplicas();
+ List<Replica> replicas = new ArrayList<>();
+ for (Slice slice : cloudClient.getClusterState().getCollection(coll)) {
+ replicas.addAll(slice.getReplicas());
+ }
Collections.shuffle(replicas, random());
return replicas.get(0);
Review Comment:
I find this painful. out of scope but trivial to change if you're in the
moood
##########
solr/core/src/test/org/apache/solr/search/TestCoordinatorRole.java:
##########
@@ -832,15 +834,22 @@ public void testMoveReplica() throws Exception {
assertEquals(DOC_PER_COLLECTION_COUNT,
response.getResults().getNumFound());
// now move the shard/replica
- String replicaName =
getCollectionState(COLLECTION_NAME).getReplicas().get(0).getName();
+ String replicaName =
+ getCollectionState(COLLECTION_NAME)
+ .getReplicaStream()
+ .findFirst()
+ .orElseThrow()
+ .getName();
String toNodeName = dataNodes.get(1);
CollectionAdminRequest.moveReplica(COLLECTION_NAME, replicaName,
toNodeName).process(client);
waitForState(
"Cannot find replica on second node yet after repliac move",
COLLECTION_NAME,
collectionState -> {
- if (collectionState.getReplicas().size() == 1) {
- Replica replica = collectionState.getReplicas().get(0);
+ List<Replica> replicas =
+
collectionState.getReplicaStream().collect(Collectors.toList());
Review Comment:
again
##########
solr/core/src/test/org/apache/solr/update/DeleteByIdWithRouterFieldTest.java:
##########
@@ -79,10 +79,12 @@ public static void setupClusterAndCollection() throws
Exception {
solrClient = cluster.getSolrClient(COLL);
ClusterState clusterState = cluster.getSolrClient().getClusterState();
- for (Replica replica : clusterState.getCollection(COLL).getReplicas()) {
- clients.add(
- new CollectionScopedSolrClient(
- cluster.getReplicaJetty(replica).getSolrClient(),
replica.getCoreName()));
+ for (Slice slice : clusterState.getCollection(COLL)) {
+ for (Replica replica : slice.getReplicas()) {
+ clients.add(
+ new CollectionScopedSolrClient(
+ cluster.getReplicaJetty(replica).getSolrClient(),
replica.getCoreName()));
+ }
Review Comment:
consider Stream to avoid double-loop
##########
solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java:
##########
@@ -400,13 +401,9 @@ public Iterator<Slice> iterator() {
return slices.values().iterator();
}
- @Deprecated // low usage and builds an ArrayList (surprising)
- public List<Replica> getReplicas() {
- List<Replica> replicas = new ArrayList<>();
- for (Slice slice : this) {
- replicas.addAll(slice.getReplicas());
- }
- return replicas;
+ /** Stream of all replicas across all slices, without allocating an
intermediate list. */
Review Comment:
> without allocating an intermediate list
The tell-tale sign of an LLM. It documents what it did *not* do that the
previous code did.
##########
solr/core/src/test/org/apache/solr/cloud/TestCloudSearcherWarming.java:
##########
@@ -214,9 +215,11 @@ public void testPeersyncFailureReplicationSuccess() throws
Exception {
// but clusterShape will also return true if replica is not live -- which
we don't want
Predicate<DocCollection> collectionStatePredicate =
collectionState -> {
- for (Replica r : collectionState.getReplicas()) {
- if (r.getNodeName().equals(oldNodeName.get())) {
- return r.getState() == Replica.State.DOWN;
+ for (Slice slice : collectionState) {
+ for (Replica r : slice.getReplicas()) {
+ if (r.getNodeName().equals(oldNodeName.get())) {
+ return r.getState() == Replica.State.DOWN;
+ }
}
}
return false;
Review Comment:
could be a Stream one-liner
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
