epugh commented on code in PR #4760:
URL: https://github.com/apache/solr/pull/4760#discussion_r3820586612
##########
solr/core/src/test/org/apache/solr/cloud/api/collections/CollectionTooManyReplicasTest.java:
##########
@@ -113,8 +113,10 @@ public void testAddTooManyReplicas() throws Exception {
"Expected to see all replicas active",
collectionName,
c -> {
- for (Replica r : c.getReplicas()) {
- if (r.getState() != Replica.State.ACTIVE) return false;
+ for (Slice s : c) {
Review Comment:
same not sure if we need this for loop?
##########
solr/core/src/test/org/apache/solr/cloud/AddReplicaTest.java:
##########
@@ -211,7 +211,13 @@ public void testAddReplicaWithUserDefinedProperties()
throws Exception {
// Verify that the new core was created with user-defined properties
coming from the request
// and inherited from the collection (the former taking precedence over
the latter).
Replica replica =
-
cloudClient.getClusterState().getCollection(collectionName).getReplicas().get(1);
+ cloudClient
+ .getClusterState()
+ .getCollection(collectionName)
+ .getReplicaStream()
+ .skip(1)
Review Comment:
We know we want to skip 1?
##########
solr/core/src/test/org/apache/solr/cloud/DistribDocExpirationUpdateProcessorTest.java:
##########
@@ -285,39 +285,41 @@ private Map<String, ReplicaData>
getTestDataForAllReplicas()
DocCollection collectionState =
cluster.getSolrClient().getClusterState().getCollection(COLLECTION);
- for (Replica replica : collectionState.getReplicas()) {
-
- String coreName = replica.getCoreName();
- try (SolrClient client = getHttpSolrClient(replica)) {
-
- ModifiableSolrParams params = new ModifiableSolrParams();
- params.set("command", "indexversion");
- params.set("_trace", "getIndexVersion");
- QueryRequest req = setAuthIfNeeded(new
QueryRequest(ReplicationHandler.PATH, params));
-
- NamedList<Object> res = client.request(req);
- assertNotNull("null response from server: " + coreName, res);
-
- Object version = res.get("indexversion");
- assertNotNull("null version from server: " + coreName, version);
- assertTrue("version isn't a long: " + coreName, version instanceof
Long);
-
- long numDocs =
- setAuthIfNeeded(
- new QueryRequest(
- params(
- "q", "*:*",
- "distrib", "false",
- "rows", "0",
- "_trace", "counting_docs")))
- .process(client)
- .getResults()
- .getNumFound();
-
- final ReplicaData data =
- new ReplicaData(replica.getShard(), coreName, (Long) version,
numDocs);
- log.info("{}", data);
- results.put(coreName, data);
+ for (Slice slice : collectionState) {
Review Comment:
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?
##########
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 : coll) {
Review Comment:
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?
##########
solr/core/src/test/org/apache/solr/cloud/api/collections/ShardSplitTest.java:
##########
@@ -137,7 +137,7 @@ private void
doSplitStaticIndexReplication(SolrIndexSplitter.SplitMethod splitMe
cloudClient
.getClusterState()
.getCollection(AbstractFullDistribZkTestBase.DEFAULT_COLLECTION);
- Replica replica = defCol.getReplicas().get(0);
+ Replica replica = defCol.getReplicaStream().findFirst().orElseThrow();
Review Comment:
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.
##########
solr/core/src/test/org/apache/solr/cloud/AddReplicaTest.java:
##########
@@ -106,7 +106,7 @@ public void testAddMultipleReplicas() throws Exception {
docCollection =
cloudClient.getClusterState().getCollectionOrNull(collection);
assertNotNull(docCollection);
// sanity check that everything is as before
- assertEquals(9, docCollection.getReplicas().size());
+ assertEquals(9, docCollection.getReplicaStream().count());
Review Comment:
I am not sure I see a big benefit in the getReplicaStream over
getReplica.stream..
##########
solr/core/src/test/org/apache/solr/cloud/AddReplicaTest.java:
##########
@@ -106,7 +106,7 @@ public void testAddMultipleReplicas() throws Exception {
docCollection =
cloudClient.getClusterState().getCollectionOrNull(collection);
assertNotNull(docCollection);
// sanity check that everything is as before
- assertEquals(9, docCollection.getReplicas().size());
+ assertEquals(9, docCollection.getReplicaStream().count());
Review Comment:
I think I was excpecting `getReplicaStream` to do more!
--
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]
