You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/11/12 20:22:49 UTC
[GitHub] [solr] HoustonPutman opened a new pull request #414: SOLR-15795: Fix replace node.
HoustonPutman opened a new pull request #414:
URL: https://github.com/apache/solr/pull/414
Add test to make sure it fails correctly.
https://issues.apache.org/jira/browse/SOLR-15795
I also want to make this better, so that if you are moving multiple replicas, the placement formulas take into account where the previous replicas were placed when deciding the next replica placement. Maybe this deserves its own Jira though.
--
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
[GitHub] [solr] HoustonPutman commented on a change in pull request #414: SOLR-15795: Fix REPLACENODE to not use source node
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #414:
URL: https://github.com/apache/solr/pull/414#discussion_r750484513
##########
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:
It's used down below to replace the assert.
--
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
[GitHub] [solr] HoustonPutman commented on a change in pull request #414: SOLR-15795: Fix REPLACENODE to not use source node
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #414:
URL: https://github.com/apache/solr/pull/414#discussion_r750488292
##########
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:
Nvm, just read the second half of your comment. Will fix.
--
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
[GitHub] [solr] HoustonPutman commented on pull request #414: SOLR-15795: Fix REPLACENODE to not use source node
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #414:
URL: https://github.com/apache/solr/pull/414#issuecomment-969173065
@sigram , you might be interested too, but I can't add you as a reviewer.
--
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
[GitHub] [solr] murblanc commented on a change in pull request #414: SOLR-15795: Fix REPLACENODE to not use source node
Posted by 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
[GitHub] [solr] HoustonPutman commented on a change in pull request #414: SOLR-15795: Fix REPLACENODE to not use source node
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #414:
URL: https://github.com/apache/solr/pull/414#discussion_r750556449
##########
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:
Good find!
--
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
[GitHub] [solr] HoustonPutman merged pull request #414: SOLR-15795: Fix REPLACENODE to not use source node
Posted by GitBox <gi...@apache.org>.
HoustonPutman merged pull request #414:
URL: https://github.com/apache/solr/pull/414
--
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
[GitHub] [solr] HoustonPutman commented on a change in pull request #414: SOLR-15795: Fix REPLACENODE to not use source node
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #414:
URL: https://github.com/apache/solr/pull/414#discussion_r750486658
##########
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:
Just remembered the reason I use this. `getLiveNodes()` returns a set, which has no equivalent to `get(0)`
--
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
[GitHub] [solr] HoustonPutman commented on a change in pull request #414: SOLR-15795: Fix REPLACENODE to not use source node
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #414:
URL: https://github.com/apache/solr/pull/414#discussion_r750559742
##########
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:
have to remove the whole try{} as well, will do.
--
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
[GitHub] [solr] madrob commented on a change in pull request #414: SOLR-15795: Fix REPLACENODE to not use source node
Posted by 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