[GitHub] [solr] madrob commented on a change in pull request #414: SOLR-15795: Fix REPLACENODE to not use source node

[GitBox <gi...@apache.org>]

madrob commented on a change in pull request #414:
URL: https://github.com/apache/solr/pull/414#discussion_r749585421



##########
File path: solr/core/src/test/org/apache/solr/cloud/ReplaceNodeTest.java
##########
@@ -210,6 +212,25 @@ public void test() throws Exception {
 
   }
 
+  @Test
+  public void testFailOnSingleNode() throws Exception {
+    configureCluster(1)
+        .addConfig("conf1", TEST_PATH().resolve("configsets").resolve("cloud-dynamic").resolve("conf"))
+        .configure();
+    String coll = "replacesinglenodetest_coll";
+    if (log.isInfoEnabled()) {
+      log.info("total_jettys: {}", cluster.getJettySolrRunners().size());
+    }
+
+    CloudSolrClient cloudClient = cluster.getSolrClient();
+    cloudClient.request(CollectionAdminRequest.createCollection(coll, "conf1", 5, 1,0,0));
+
+    cluster.waitForActiveCollection(coll, 5, 5);
+
+    String liveNode = cloudClient.getZkStateReader().getClusterState().getLiveNodes().stream().findAny().get();

Review comment:
       I think the typical pattern is to `get(0)` if we have a list or `iterator().next()` for a collection. Stream seems somewhat heavy here.

##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java
##########
@@ -356,6 +356,26 @@ public int weight() {
     return createNodeList; // unmodified, but return for inline use
   }
 
+  // throw an exception if all nodes in the supplied list are not live.
+  // Empty list will also fail.
+  // Returns the input
+  private static List<String> checkAnyLiveNodes(List<String> createNodeList, ClusterState clusterState) {
+    Set<String> liveNodes = clusterState.getLiveNodes();
+    if (createNodeList == null) {
+      createNodeList = Collections.emptyList();
+    }
+    boolean anyLiveNodes = false;
+    for (String node : createNodeList) {
+      anyLiveNodes |= liveNodes.contains(node);
+    }
+    if (!anyLiveNodes) {
+      throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
+          "None of the node(s) specified " + createNodeList + " are not currently active in "
+              + createNodeList + ", no action taken.");
+    }
+    return createNodeList; // unmodified, but return for inline use. Only modified if empty, and that will throw an error

Review comment:
       is this meaningful? I don't see us using it yet in the patch.




-- 
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: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org