dsmiley commented on code in PR #3834:
URL: https://github.com/apache/solr/pull/3834#discussion_r2505104409
##########
solr/core/src/java/org/apache/solr/update/processor/RoutedAliasUpdateProcessor.java:
##########
@@ -267,13 +267,13 @@ private List<SolrCmdDistributor.Node>
lookupShardLeadersOfCollections() {
}
private SolrCmdDistributor.Node lookupShardLeaderOfCollection(String
collection) {
- final List<Slice> activeSlices =
- new
ArrayList<>(zkController.getClusterState().getCollection(collection).getActiveSlices());
+ final Collection<Slice> activeSlices =
+
zkController.getClusterState().getCollection(collection).getActiveSlices();
if (activeSlices.isEmpty()) {
throw new SolrException(
SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Cannot route to
collection " + collection);
}
- final Slice slice = activeSlices.getFirst();
+ final Slice slice = activeSlices.stream().findAny().orElseThrow();
Review Comment:
Don't actually need an isEmpty check; can instead use the feature of
Optional's `orElseThrow` that is overloaded to throw an exception.
##########
solr/core/src/test/org/apache/solr/cloud/MigrateReplicasTest.java:
##########
@@ -184,17 +183,14 @@ public void test() throws Exception {
}
// make sure all newly created replicas on node are active
List<Replica> newReplicas =
collection.getReplicasOnNode(nodeToBeDecommissioned);
- assertNotNull("There should be replicas on the migrated-to node",
newReplicas);
assertFalse("There should be replicas on the migrated-to node",
newReplicas.isEmpty());
for (Replica r : newReplicas) {
assertEquals(r.toString(), Replica.State.ACTIVE, r.getState());
}
// make sure all replicas on emptyNode are not active
List<Replica> replicas = collection.getReplicasOnNode(emptyNode);
Review Comment:
nit / taste: I love inlining to avoid needlessly declaring a variable
(giving it a name and long scope); but I leave it to you (many places in the PR
where we subsequently loop)
##########
solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java:
##########
@@ -314,9 +315,9 @@ public Map<String, Slice> getActiveSlicesMap() {
return activeSlices;
}
- /** Get the list of replicas hosted on the given node or <code>null</code>
if none. */
+ /** Get the list of replicas hosted on the given node, or an empty list if
none. */
public List<Replica> getReplicasOnNode(String nodeName) {
- return nodeNameReplicas.get(nodeName);
+ return nodeNameReplicas.getOrDefault(nodeName, Collections.emptyList());
Review Comment:
This is fine but nowadays I think we'd prefer the equivalent `List.of()`
--
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]
