From c9cf57fc2838b25b425da9c7039cbeb580d374bb Mon Sep 17 00:00:00 2001 From: Shalin Shekhar Mangar Date: Sat, 15 Jul 2017 09:05:33 +0530 Subject: [PATCH] SOLR-11068: MOVEREPLICA and REPLACENODE API parameter names are now 'sourceNode' and 'targetNode'. The old names viz. 'fromNode' for MOVEREPLICA and 'source', 'target' for REPLACENODE have been deprecated --- solr/CHANGES.txt | 7 +++ .../org/apache/solr/cloud/MoveReplicaCmd.java | 11 ++-- .../org/apache/solr/cloud/ReplaceNodeCmd.java | 9 ++-- .../handler/admin/CollectionsHandler.java | 18 +++++-- .../apache/solr/cloud/MoveReplicaTest.java | 50 ++++++++++++++++++- .../apache/solr/cloud/ReplaceNodeTest.java | 26 +++++++++- solr/solr-ref-guide/src/collections-api.adoc | 12 ++--- .../autoscaling/MoveReplicaSuggester.java | 8 +-- .../solrj/request/CollectionAdminRequest.java | 27 +++++----- .../solr/common/params/CollectionParams.java | 9 ++++ 10 files changed, 140 insertions(+), 37 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index b2bc5478fea..d615d3e54ae 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -211,6 +211,10 @@ Upgrading from Solr 6.x * StandardRequestHandler is deprecated. Simply use SearchHandler instead. +* The parameter names 'fromNode' for MOVEREPLICA and 'source', 'target' for REPLACENODE have been deprecated and + replaced with 'sourceNode' and 'targetNode' instead. The old names will continue to work for back-compatibility + but they will be removed in 8.0. See SOLR-11068 for more details. + New Features ---------------------- * SOLR-9857, SOLR-9858: Collect aggregated metrics from nodes and shard leaders in overseer. (ab) @@ -503,6 +507,9 @@ Other Changes * SOLR-10796: TestPointFields: increase randomized testing of non-trivial values. (Steve Rowe) +* SOLR-11068: MOVEREPLICA and REPLACENODE API parameter names are now 'sourceNode' and 'targetNode'. The old names + viz. 'fromNode' for MOVEREPLICA and 'source', 'target' for REPLACENODE have been deprecated. (shalin) + ================== 6.7.0 ================== Consult the LUCENE_CHANGES.txt file for additional, low level, changes in this release. diff --git a/solr/core/src/java/org/apache/solr/cloud/MoveReplicaCmd.java b/solr/core/src/java/org/apache/solr/cloud/MoveReplicaCmd.java index 117edf92d18..19075e5d412 100644 --- a/solr/core/src/java/org/apache/solr/cloud/MoveReplicaCmd.java +++ b/solr/core/src/java/org/apache/solr/cloud/MoveReplicaCmd.java @@ -31,6 +31,7 @@ import org.apache.solr.common.cloud.DocCollection; import org.apache.solr.common.cloud.Replica; import org.apache.solr.common.cloud.Slice; import org.apache.solr.common.cloud.ZkNodeProps; +import org.apache.solr.common.params.CollectionParams; import org.apache.solr.common.params.CoreAdminParams; import org.apache.solr.common.util.NamedList; import org.apache.solr.common.util.Utils; @@ -81,20 +82,22 @@ public class MoveReplicaCmd implements Cmd{ "Collection: " + collection + " replica: " + replicaName + " does not exist"); } } else { - ocmh.checkRequired(message, SHARD_ID_PROP, "fromNode"); - String fromNode = message.getStr("fromNode"); + String sourceNode = message.getStr(CollectionParams.SOURCE_NODE, message.getStr(CollectionParams.FROM_NODE)); + if (sourceNode == null) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "sourceNode is a required param" ); + } String shardId = message.getStr(SHARD_ID_PROP); Slice slice = clusterState.getCollection(collection).getSlice(shardId); List sliceReplicas = new ArrayList<>(slice.getReplicas()); Collections.shuffle(sliceReplicas, RANDOM); for (Replica r : slice.getReplicas()) { - if (r.getNodeName().equals(fromNode)) { + if (r.getNodeName().equals(sourceNode)) { replica = r; } } if (replica == null) { throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, - "Collection: " + collection + " node: " + fromNode + " do not have any replica belong to shard: " + shardId); + "Collection: " + collection + " node: " + sourceNode + " do not have any replica belong to shard: " + shardId); } } diff --git a/solr/core/src/java/org/apache/solr/cloud/ReplaceNodeCmd.java b/solr/core/src/java/org/apache/solr/cloud/ReplaceNodeCmd.java index ba60908a8d8..ef3fd89ec4e 100644 --- a/solr/core/src/java/org/apache/solr/cloud/ReplaceNodeCmd.java +++ b/solr/core/src/java/org/apache/solr/cloud/ReplaceNodeCmd.java @@ -37,6 +37,7 @@ import org.apache.solr.common.cloud.Replica; import org.apache.solr.common.cloud.Slice; import org.apache.solr.common.cloud.ZkNodeProps; import org.apache.solr.common.cloud.ZkStateReader; +import org.apache.solr.common.params.CollectionParams; import org.apache.solr.common.params.CoreAdminParams; import org.apache.solr.common.util.NamedList; import org.apache.zookeeper.KeeperException; @@ -59,9 +60,11 @@ public class ReplaceNodeCmd implements OverseerCollectionMessageHandler.Cmd { @Override public void call(ClusterState state, ZkNodeProps message, NamedList results) throws Exception { ZkStateReader zkStateReader = ocmh.zkStateReader; - ocmh.checkRequired(message, "source", "target"); - String source = message.getStr("source"); - String target = message.getStr("target"); + String source = message.getStr(CollectionParams.SOURCE_NODE, message.getStr("source")); + String target = message.getStr(CollectionParams.TARGET_NODE, message.getStr("target")); + if (source == null || target == null) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "sourceNode and targetNode are required params" ); + } String async = message.getStr("async"); int timeout = message.getInt("timeout", 10 * 60); // 10 minutes boolean parallel = message.getBool("parallel", false); diff --git a/solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java b/solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java index a4155ea6b4a..3afad2fb7b7 100644 --- a/solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java +++ b/solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java @@ -882,14 +882,26 @@ public class CollectionsHandler extends RequestHandlerBase implements Permission rsp.add(SolrSnapshotManager.SNAPSHOTS_INFO, snapshots); return null; }), - REPLACENODE_OP(REPLACENODE, (req, rsp, h) -> req.getParams().required().getAll(req.getParams().getAll(null, "parallel"), "source", "target")), + REPLACENODE_OP(REPLACENODE, (req, rsp, h) -> { + SolrParams params = req.getParams(); + String sourceNode = params.get(CollectionParams.SOURCE_NODE, params.get("source")); + if (sourceNode == null) { + throw new SolrException(ErrorCode.BAD_REQUEST, CollectionParams.SOURCE_NODE + " is a require parameter"); + } + String targetNode = params.get(CollectionParams.TARGET_NODE, params.get("target")); + if (targetNode == null) { + throw new SolrException(ErrorCode.BAD_REQUEST, CollectionParams.TARGET_NODE + " is a require parameter"); + } + return params.getAll(null, "source", "target", CollectionParams.SOURCE_NODE, CollectionParams.TARGET_NODE); + }), MOVEREPLICA_OP(MOVEREPLICA, (req, rsp, h) -> { Map map = req.getParams().required().getAll(null, COLLECTION_PROP); return req.getParams().getAll(map, - "fromNode", - "targetNode", + CollectionParams.FROM_NODE, + CollectionParams.SOURCE_NODE, + CollectionParams.TARGET_NODE, "replica", "shard"); }), diff --git a/solr/core/src/test/org/apache/solr/cloud/MoveReplicaTest.java b/solr/core/src/test/org/apache/solr/cloud/MoveReplicaTest.java index 930e8ee7feb..3e65850fb88 100644 --- a/solr/core/src/test/org/apache/solr/cloud/MoveReplicaTest.java +++ b/solr/core/src/test/org/apache/solr/cloud/MoveReplicaTest.java @@ -35,6 +35,9 @@ import org.apache.solr.client.solrj.response.RequestStatusState; import org.apache.solr.common.cloud.DocCollection; import org.apache.solr.common.cloud.Replica; import org.apache.solr.common.cloud.Slice; +import org.apache.solr.common.params.CollectionParams; +import org.apache.solr.common.params.ModifiableSolrParams; +import org.apache.solr.common.params.SolrParams; import org.junit.BeforeClass; import org.junit.Test; import org.slf4j.Logger; @@ -86,7 +89,7 @@ public class MoveReplicaTest extends SolrCloudTestCase { } } - CollectionAdminRequest.MoveReplica moveReplica = new CollectionAdminRequest.MoveReplica(coll, replica.getName(), targetNode); + CollectionAdminRequest.MoveReplica moveReplica = createMoveReplicaRequest(coll, replica, targetNode); moveReplica.processAsync("000", cloudClient); CollectionAdminRequest.RequestStatus requestStatus = CollectionAdminRequest.requestStatus("000"); // wait for async request success @@ -141,7 +144,7 @@ public class MoveReplicaTest extends SolrCloudTestCase { } assertTrue("replica never fully recovered", recovered); - moveReplica = new CollectionAdminRequest.MoveReplica(coll, shardId, targetNode, replica.getNodeName()); + moveReplica = createMoveReplicaRequest(coll, replica, targetNode, shardId); moveReplica.process(cloudClient); checkNumOfCores(cloudClient, replica.getNodeName(), 1); // wait for recovery @@ -181,6 +184,49 @@ public class MoveReplicaTest extends SolrCloudTestCase { assertTrue("replica never fully recovered", recovered); } + private CollectionAdminRequest.MoveReplica createMoveReplicaRequest(String coll, Replica replica, String targetNode, String shardId) { + if (random().nextBoolean()) { + return new CollectionAdminRequest.MoveReplica(coll, shardId, targetNode, replica.getNodeName()); + } else { + // for backcompat testing of SOLR-11068 + // todo remove in solr 8.0 + return new BackCompatMoveReplicaRequest(coll, shardId, targetNode, replica.getNodeName()); + } + } + + private CollectionAdminRequest.MoveReplica createMoveReplicaRequest(String coll, Replica replica, String targetNode) { + if (random().nextBoolean()) { + return new CollectionAdminRequest.MoveReplica(coll, replica.getName(), targetNode); + } else { + // for backcompat testing of SOLR-11068 + // todo remove in solr 8.0 + return new BackCompatMoveReplicaRequest(coll, replica.getName(), targetNode); + } + } + + /** + * Added for backcompat testing + * todo remove in solr 8.0 + */ + static class BackCompatMoveReplicaRequest extends CollectionAdminRequest.MoveReplica { + public BackCompatMoveReplicaRequest(String collection, String replica, String targetNode) { + super(collection, replica, targetNode); + } + + public BackCompatMoveReplicaRequest(String collection, String shard, String sourceNode, String targetNode) { + super(collection, shard, sourceNode, targetNode); + } + + @Override + public SolrParams getParams() { + ModifiableSolrParams params = (ModifiableSolrParams) super.getParams(); + if (randomlyMoveReplica) { + params.set(CollectionParams.FROM_NODE, sourceNode); + } + return params; + } + } + private Replica getRandomReplica(String coll, CloudSolrClient cloudClient) { List replicas = cloudClient.getZkStateReader().getClusterState().getCollection(coll).getReplicas(); Collections.shuffle(replicas, random()); diff --git a/solr/core/src/test/org/apache/solr/cloud/ReplaceNodeTest.java b/solr/core/src/test/org/apache/solr/cloud/ReplaceNodeTest.java index edbeb501acc..db6221224fd 100644 --- a/solr/core/src/test/org/apache/solr/cloud/ReplaceNodeTest.java +++ b/solr/core/src/test/org/apache/solr/cloud/ReplaceNodeTest.java @@ -33,6 +33,9 @@ import org.apache.solr.client.solrj.response.RequestStatusState; import org.apache.solr.common.cloud.DocCollection; import org.apache.solr.common.cloud.Replica; import org.apache.solr.common.cloud.Slice; +import org.apache.solr.common.params.CollectionParams; +import org.apache.solr.common.params.ModifiableSolrParams; +import org.apache.solr.common.params.SolrParams; import org.apache.solr.common.util.StrUtils; import org.junit.BeforeClass; import org.junit.Test; @@ -80,7 +83,7 @@ public class ReplaceNodeTest extends SolrCloudTestCase { create.setCreateNodeSet(StrUtils.join(l, ',')).setMaxShardsPerNode(3); cloudClient.request(create); log.info("excluded_node : {} ", emptyNode); - new CollectionAdminRequest.ReplaceNode(node2bdecommissioned, emptyNode).processAsync("000", cloudClient); + createReplaceNodeRequest(node2bdecommissioned, emptyNode, null).processAsync("000", cloudClient); CollectionAdminRequest.RequestStatus requestStatus = CollectionAdminRequest.requestStatus("000"); boolean success = false; for (int i = 0; i < 300; i++) { @@ -99,7 +102,7 @@ public class ReplaceNodeTest extends SolrCloudTestCase { } //let's do it back - new CollectionAdminRequest.ReplaceNode(emptyNode, node2bdecommissioned).setParallel(Boolean.TRUE).processAsync("001", cloudClient); + createReplaceNodeRequest(emptyNode, node2bdecommissioned, Boolean.TRUE).processAsync("001", cloudClient); requestStatus = CollectionAdminRequest.requestStatus("001"); for (int i = 0; i < 200; i++) { @@ -125,4 +128,23 @@ public class ReplaceNodeTest extends SolrCloudTestCase { assertEquals(create.getNumPullReplicas().intValue(), s.getReplicas(EnumSet.of(Replica.Type.PULL)).size()); } } + + private CollectionAdminRequest.AsyncCollectionAdminRequest createReplaceNodeRequest(String sourceNode, String targetNode, Boolean parallel) { + if (random().nextBoolean()) { + return new CollectionAdminRequest.ReplaceNode(sourceNode, targetNode).setParallel(parallel); + } else { + // test back compat with old param names + // todo remove in solr 8.0 + return new CollectionAdminRequest.AsyncCollectionAdminRequest(CollectionParams.CollectionAction.REPLACENODE) { + @Override + public SolrParams getParams() { + ModifiableSolrParams params = (ModifiableSolrParams) super.getParams(); + params.set("source", sourceNode); + params.set("target", targetNode); + if (parallel != null) params.set("parallel", parallel.toString()); + return params; + } + }; + } + } } diff --git a/solr/solr-ref-guide/src/collections-api.adoc b/solr/solr-ref-guide/src/collections-api.adoc index 662c5fb4cf6..7e6742b8e7b 100644 --- a/solr/solr-ref-guide/src/collections-api.adoc +++ b/solr/solr-ref-guide/src/collections-api.adoc @@ -1832,14 +1832,14 @@ This command recreates replicas in one node (the source) to another node (the ta For source replicas that are also shard leaders the operation will wait for the number of seconds set with the `timeout` parameter to make sure there's an active replica that can become a leader (either an existing replica becoming a leader or the new replica completing recovery and becoming a leader). -`/admin/collections?action=REPLACENODE&source=_source-node_&target=_target-node_` +`/admin/collections?action=REPLACENODE&sourceNode=_source-node_&targetNode=_target-node_` === REPLACENODE Parameters -`source`:: +`sourceNode`:: The source node from which the replicas need to be copied from. This parameter is required. -`target`:: +`targetNode`:: The target node where replicas will be copied. This parameter is required. `parallel`:: @@ -1860,7 +1860,7 @@ This operation does not hold necessary locks on the replicas that belong to on t This command moves a replica from one node to a new node. In case of shared filesystems the `dataDir` will be reused. -`/admin/collections?action=MOVEREPLICA&collection=collection&shard=shard&replica=replica&node=nodeName&toNode=nodeName` +`/admin/collections?action=MOVEREPLICA&collection=collection&shard=shard&replica=replica&sourceNode=nodeName&targetNode=nodeName` === MOVEREPLICA Parameters @@ -1873,10 +1873,10 @@ The name of the shard that the replica belongs to. This parameter is required. `replica`:: The name of the replica. This parameter is required. -`node`:: +`sourceNode`:: The name of the node that contains the replica. This parameter is required. -`toNode`:: +`targetNode`:: The name of the destination node. This parameter is required. `async`:: diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/MoveReplicaSuggester.java b/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/MoveReplicaSuggester.java index bf9c284ad02..e97cb0e392b 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/MoveReplicaSuggester.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/MoveReplicaSuggester.java @@ -39,7 +39,7 @@ public class MoveReplicaSuggester extends Suggester { //iterate through elements and identify the least loaded List leastSeriousViolation = null; Integer targetNodeIndex = null; - Integer fromNodeIndex = null; + Integer sourceNodeIndex = null; ReplicaInfo fromReplicaInfo = null; for (Pair fromReplica : getValidReplicas(true, true, -1)) { Row fromRow = fromReplica.second(); @@ -64,13 +64,13 @@ public class MoveReplicaSuggester extends Suggester { if (!containsNewErrors(errs) && isLessSerious(errs, leastSeriousViolation)) { leastSeriousViolation = errs; targetNodeIndex = j; - fromNodeIndex = i; + sourceNodeIndex = i; fromReplicaInfo = replicaInfo; } } } - if (targetNodeIndex != null && fromNodeIndex != null) { - getMatrix().set(fromNodeIndex, getMatrix().get(fromNodeIndex).removeReplica(fromReplicaInfo.collection, fromReplicaInfo.shard).first()); + if (targetNodeIndex != null && sourceNodeIndex != null) { + getMatrix().set(sourceNodeIndex, getMatrix().get(sourceNodeIndex).removeReplica(fromReplicaInfo.collection, fromReplicaInfo.shard).first()); getMatrix().set(targetNodeIndex, getMatrix().get(targetNodeIndex).addReplica(fromReplicaInfo.collection, fromReplicaInfo.shard)); return new CollectionAdminRequest.MoveReplica( fromReplicaInfo.collection, diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java b/solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java index 88100f31aaf..16f2e2ea766 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java @@ -37,6 +37,7 @@ import org.apache.solr.common.cloud.ImplicitDocRouter; import org.apache.solr.common.cloud.Replica; import org.apache.solr.common.cloud.ZkStateReader; import org.apache.solr.common.params.CollectionAdminParams; +import org.apache.solr.common.params.CollectionParams; import org.apache.solr.common.params.CollectionParams.CollectionAction; import org.apache.solr.common.params.CommonAdminParams; import org.apache.solr.common.params.CoreAdminParams; @@ -547,7 +548,7 @@ public abstract class CollectionAdminRequest } public static class ReplaceNode extends AsyncCollectionAdminRequest { - String source, target; + String sourceNode, targetNode; Boolean parallel; /** @@ -556,8 +557,8 @@ public abstract class CollectionAdminRequest */ public ReplaceNode(String source, String target) { super(CollectionAction.REPLACENODE); - this.source = checkNotNull("source",source); - this.target = checkNotNull("target",target); + this.sourceNode = checkNotNull(CollectionParams.SOURCE_NODE, source); + this.targetNode = checkNotNull(CollectionParams.TARGET_NODE, target); } public ReplaceNode setParallel(Boolean flag) { @@ -568,8 +569,8 @@ public abstract class CollectionAdminRequest @Override public SolrParams getParams() { ModifiableSolrParams params = (ModifiableSolrParams) super.getParams(); - params.set("source", source); - params.set("target", target); + params.set(CollectionParams.SOURCE_NODE, sourceNode); + params.set(CollectionParams.TARGET_NODE, targetNode); if (parallel != null) params.set("parallel", parallel.toString()); return params; } @@ -577,9 +578,9 @@ public abstract class CollectionAdminRequest } public static class MoveReplica extends AsyncCollectionAdminRequest { - String collection, replica, targetNode; - String shard, fromNode; - boolean randomlyMoveReplica; + protected String collection, replica, targetNode; + protected String shard, sourceNode; + protected boolean randomlyMoveReplica; public MoveReplica(String collection, String replica, String targetNode) { super(CollectionAction.MOVEREPLICA); @@ -589,12 +590,12 @@ public abstract class CollectionAdminRequest this.randomlyMoveReplica = false; } - public MoveReplica(String collection, String shard, String fromNode, String targetNode) { + public MoveReplica(String collection, String shard, String sourceNode, String targetNode) { super(CollectionAction.MOVEREPLICA); this.collection = checkNotNull("collection",collection); this.shard = checkNotNull("shard",shard); - this.fromNode = checkNotNull("fromNode",fromNode); - this.targetNode = checkNotNull("targetNode",targetNode); + this.sourceNode = checkNotNull(CollectionParams.SOURCE_NODE, sourceNode); + this.targetNode = checkNotNull(CollectionParams.TARGET_NODE, targetNode); this.randomlyMoveReplica = true; } @@ -602,10 +603,10 @@ public abstract class CollectionAdminRequest public SolrParams getParams() { ModifiableSolrParams params = (ModifiableSolrParams) super.getParams(); params.set("collection", collection); - params.set("targetNode", targetNode); + params.set(CollectionParams.TARGET_NODE, targetNode); if (randomlyMoveReplica) { params.set("shard", shard); - params.set("fromNode", fromNode); + params.set(CollectionParams.SOURCE_NODE, sourceNode); } else { params.set("replica", replica); } diff --git a/solr/solrj/src/java/org/apache/solr/common/params/CollectionParams.java b/solr/solrj/src/java/org/apache/solr/common/params/CollectionParams.java index d79fafa3efd..069f1e5b386 100644 --- a/solr/solrj/src/java/org/apache/solr/common/params/CollectionParams.java +++ b/solr/solrj/src/java/org/apache/solr/common/params/CollectionParams.java @@ -31,6 +31,15 @@ public interface CollectionParams { String ACTION = "action"; String NAME = "name"; + /** + * @deprecated use {@link #SOURCE_NODE} instead + */ + @Deprecated + String FROM_NODE = "fromNode"; + + String SOURCE_NODE = "sourceNode"; + String TARGET_NODE = "targetNode"; + enum LockLevel { CLUSTER(0),