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

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

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



##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/ReplaceNodeCmd.java
##########
@@ -115,7 +119,7 @@ public void call(ClusterState state, ZkNodeProps message, NamedList<Object> resu
               .assignNrtReplicas(numNrtReplicas)
               .assignTlogReplicas(numTlogReplicas)
               .assignPullReplicas(numPullReplicas)
-              .onNodes(new ArrayList<>(ccc.getSolrCloudManager().getClusterStateProvider().getLiveNodes()))
+              .onNodes(ccc.getSolrCloudManager().getClusterStateProvider().getLiveNodes().stream().filter(node -> !node.equals(source)).collect(Collectors.toList()))

Review comment:
       Can you please remove line 185 and the empty finally below while you're changing this file (unrelated to your changes and Github doesn't allow commenting there)

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

Review comment:
       Message should be reworded. "not" likely removed and `createNodeList` output only once (second instance should be `liveNodes`).
   Maybe "not currently active in" replaced by "active nodes"?
   

##########
File path: solr/core/src/test/org/apache/solr/cloud/ReplaceNodeTest.java
##########
@@ -158,12 +165,7 @@ public void test() throws Exception {
     // make sure all newly created replicas on node are active
     List<Replica> newReplicas = collection.getReplicas(node2bdecommissioned);
     replicas.forEach(r -> {

Review comment:
       Curly brackets are not needed:
   
   `replicas.forEach(r -> newReplicas.removeIf(nr -> nr.getName().equals(r.getName())));`




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