gerlowskija commented on code in PR #1115:
URL: https://github.com/apache/solr/pull/1115#discussion_r1005806358
##########
solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java:
##########
@@ -1791,14 +1794,17 @@ public Map<String, Object> execute(
REPLACENODE_OP(
REPLACENODE,
(req, rsp, h) -> {
- return copy(
- req.getParams(),
- null,
- "source", // legacy
- "target", // legacy
- WAIT_FOR_FINAL_STATE,
- CollectionParams.SOURCE_NODE,
- CollectionParams.TARGET_NODE);
+ final SolrParams params = req.getParams();
+ final RequiredSolrParams requiredParams = req.getParams().required();
+ final ReplaceNodeAPI.ReplaceNodeRequestBody requestBody =
+ new ReplaceNodeAPI.ReplaceNodeRequestBody();
+ requestBody.targetNodeName = params.get(TARGET_NODE);
+ requestBody.waitForFinalState = params.getBool(WAIT_FOR_FINAL_STATE,
false);
Review Comment:
[0] Prior to this PR, if the user didn't explicitly specify a
waitForFinalState value, the created "message" wouldn't contain that key at all.
The use of `getBool(key, defaultVal)` here means that the constructed RNRB
will _always_ have a waitForFinalState value that gets added to the "message"
that the v2 code builds. This isn't a problem necessarily; the docs say that
this parameter defaults to "false" - so this shouldn't change any behavior.
But it is something to keep an eye on if test failures crop up later.
If you wanted to be more conservative though, you could drop the default-val
here to keep closer in line with the existing behavior.
##########
solr/core/src/java/org/apache/solr/handler/admin/api/ReplaceNodeAPI.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.handler.admin.api;
+
+import static
org.apache.solr.client.solrj.impl.BinaryResponseParser.BINARY_CONTENT_TYPE_V2;
+import static org.apache.solr.cloud.Overseer.QUEUE_OPERATION;
+import static org.apache.solr.common.params.CollectionParams.SOURCE_NODE;
+import static org.apache.solr.common.params.CollectionParams.TARGET_NODE;
+import static
org.apache.solr.common.params.CommonAdminParams.WAIT_FOR_FINAL_STATE;
+import static
org.apache.solr.handler.admin.CollectionsHandler.DEFAULT_COLLECTION_OP_TIMEOUT;
+import static
org.apache.solr.security.PermissionNameProvider.Name.COLL_EDIT_PERM;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import io.swagger.v3.oas.annotations.Parameter;
+import io.swagger.v3.oas.annotations.media.Schema;
+import io.swagger.v3.oas.annotations.parameters.RequestBody;
+import java.util.HashMap;
+import java.util.Map;
+import javax.inject.Inject;
+import javax.ws.rs.POST;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import org.apache.solr.client.solrj.SolrResponse;
+import org.apache.solr.common.cloud.ZkNodeProps;
+import org.apache.solr.common.params.CollectionParams;
+import org.apache.solr.common.params.CollectionParams.CollectionAction;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.handler.admin.CollectionsHandler;
+import org.apache.solr.jersey.JacksonReflectMapWriter;
+import org.apache.solr.jersey.PermissionName;
+import org.apache.solr.jersey.SolrJerseyResponse;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+
+/**
+ * V2 API for recreating replicas in one node (the source) on another node(s)
(the target).
+ *
+ * <p>This API is analogous to the v1 /admin/collections?action=REPLACENODE
command.
+ */
+@Path("cluster/nodes/{sourceNodeName}/commands/replace/")
+public class ReplaceNodeAPI extends AdminAPIBase {
+
+ @Inject
+ public ReplaceNodeAPI(
+ CoreContainer coreContainer,
+ SolrQueryRequest solrQueryRequest,
+ SolrQueryResponse solrQueryResponse) {
+ super(coreContainer, solrQueryRequest, solrQueryResponse);
+ }
+
+ @POST
+ @Produces({"application/json", "application/xml", BINARY_CONTENT_TYPE_V2})
+ @PermissionName(COLL_EDIT_PERM)
+ public SolrJerseyResponse replaceNode(
+ @Parameter(description = "The name of the node to be replaced.",
required = true)
+ @PathParam("sourceNodeName")
+ String nodeName,
+ @RequestBody(description = "Contains user provided parameters", required
= true)
+ ReplaceNodeRequestBody requestBody)
+ throws Exception {
+ final SolrJerseyResponse response =
instantiateJerseyResponse(SolrJerseyResponse.class);
+ final CoreContainer coreContainer =
fetchAndValidateZooKeeperAwareCoreContainer();
+ /** TODO Record node for log and tracing */
+ final ZkNodeProps remoteMessage = createRemoteMessage(nodeName,
requestBody);
+ final SolrResponse remoteResponse =
+ CollectionsHandler.submitCollectionApiCommand(
+ coreContainer,
+ coreContainer.getDistributedCollectionCommandRunner(),
+ remoteMessage,
+ CollectionParams.CollectionAction.REPLACENODE,
+ DEFAULT_COLLECTION_OP_TIMEOUT);
+ if (remoteResponse.getException() != null) {
+ throw remoteResponse.getException();
+ }
+
+ disableResponseCaching();
+ return response;
+ }
+
+ public ZkNodeProps createRemoteMessage(String nodeName,
ReplaceNodeRequestBody requestBody) {
+ final Map<String, Object> remoteMessage = new HashMap<>();
+ remoteMessage.put(SOURCE_NODE, nodeName);
+ remoteMessage.put(TARGET_NODE, requestBody.targetNodeName);
+ remoteMessage.put(WAIT_FOR_FINAL_STATE, requestBody.waitForFinalState);
+ remoteMessage.put(QUEUE_OPERATION, CollectionAction.REPLACENODE.toLower());
+
+ return new ZkNodeProps(remoteMessage);
+ }
+
+ public static class ReplaceNodeRequestBody implements
JacksonReflectMapWriter {
+
+ public ReplaceNodeRequestBody() {}
+
+ public ReplaceNodeRequestBody(String targetNodeName, Boolean
waitForFinalState) {
+ this.targetNodeName = targetNodeName;
+ this.waitForFinalState = waitForFinalState;
+ }
+
+ @Schema(description = "The name of the targetNode.")
+ @JsonProperty("targetNodeName")
+ public String targetNodeName;
+
+ @JsonProperty("waitForFinalState")
Review Comment:
[0] The `@Schema` annotations you see on this code end up used as
descriptions in the OpenAPI spec that Solr generates as part of its build.
If you're not familiar with OpenAPI, it's really awesome! It provides
tooling you can use to generate a "spec" - a machine-readable description of
your API. And then other tooling can take that spec and do all sorts of things
with it, like generating clients for various languages.
Right now, Solr's set up to generate a spec (run `./gradlew resolve` and
take a look at `solr/core/build/generated/openapi/openapi.json`). We haven't
started using the spec yet, but it's on my roadmap to set us up to generate
Solr clients for Python, golang, etc.
Anyway, this is all to say: these `@Schema` annotations don't seem like they
do much now, but they'll be super helpful to have in place once we start using
the spec.
You don't need to have them everywhere if you're not sure what to say about
certain parameters. But if you want to include them, you can always copy the
snippet in the ref guide that describes the parameter.
##########
solr/core/src/java/org/apache/solr/handler/admin/api/ReplaceNodeAPI.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.handler.admin.api;
+
+import static
org.apache.solr.client.solrj.impl.BinaryResponseParser.BINARY_CONTENT_TYPE_V2;
+import static org.apache.solr.cloud.Overseer.QUEUE_OPERATION;
+import static org.apache.solr.common.params.CollectionParams.SOURCE_NODE;
+import static org.apache.solr.common.params.CollectionParams.TARGET_NODE;
+import static
org.apache.solr.common.params.CommonAdminParams.WAIT_FOR_FINAL_STATE;
+import static
org.apache.solr.handler.admin.CollectionsHandler.DEFAULT_COLLECTION_OP_TIMEOUT;
+import static
org.apache.solr.security.PermissionNameProvider.Name.COLL_EDIT_PERM;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import io.swagger.v3.oas.annotations.Parameter;
+import io.swagger.v3.oas.annotations.media.Schema;
+import io.swagger.v3.oas.annotations.parameters.RequestBody;
+import java.util.HashMap;
+import java.util.Map;
+import javax.inject.Inject;
+import javax.ws.rs.POST;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import org.apache.solr.client.solrj.SolrResponse;
+import org.apache.solr.common.cloud.ZkNodeProps;
+import org.apache.solr.common.params.CollectionParams;
+import org.apache.solr.common.params.CollectionParams.CollectionAction;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.handler.admin.CollectionsHandler;
+import org.apache.solr.jersey.JacksonReflectMapWriter;
+import org.apache.solr.jersey.PermissionName;
+import org.apache.solr.jersey.SolrJerseyResponse;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+
+/**
+ * V2 API for recreating replicas in one node (the source) on another node(s)
(the target).
+ *
+ * <p>This API is analogous to the v1 /admin/collections?action=REPLACENODE
command.
+ */
+@Path("cluster/nodes/{sourceNodeName}/commands/replace/")
+public class ReplaceNodeAPI extends AdminAPIBase {
+
+ @Inject
+ public ReplaceNodeAPI(
+ CoreContainer coreContainer,
+ SolrQueryRequest solrQueryRequest,
+ SolrQueryResponse solrQueryResponse) {
+ super(coreContainer, solrQueryRequest, solrQueryResponse);
+ }
+
+ @POST
+ @Produces({"application/json", "application/xml", BINARY_CONTENT_TYPE_V2})
+ @PermissionName(COLL_EDIT_PERM)
+ public SolrJerseyResponse replaceNode(
+ @Parameter(description = "The name of the node to be replaced.",
required = true)
+ @PathParam("sourceNodeName")
+ String nodeName,
+ @RequestBody(description = "Contains user provided parameters", required
= true)
+ ReplaceNodeRequestBody requestBody)
+ throws Exception {
+ final SolrJerseyResponse response =
instantiateJerseyResponse(SolrJerseyResponse.class);
+ final CoreContainer coreContainer =
fetchAndValidateZooKeeperAwareCoreContainer();
+ /** TODO Record node for log and tracing */
+ final ZkNodeProps remoteMessage = createRemoteMessage(nodeName,
requestBody);
+ final SolrResponse remoteResponse =
+ CollectionsHandler.submitCollectionApiCommand(
+ coreContainer,
+ coreContainer.getDistributedCollectionCommandRunner(),
+ remoteMessage,
+ CollectionParams.CollectionAction.REPLACENODE,
+ DEFAULT_COLLECTION_OP_TIMEOUT);
+ if (remoteResponse.getException() != null) {
+ throw remoteResponse.getException();
+ }
+
+ disableResponseCaching();
+ return response;
+ }
+
+ public ZkNodeProps createRemoteMessage(String nodeName,
ReplaceNodeRequestBody requestBody) {
+ final Map<String, Object> remoteMessage = new HashMap<>();
+ remoteMessage.put(SOURCE_NODE, nodeName);
+ remoteMessage.put(TARGET_NODE, requestBody.targetNodeName);
+ remoteMessage.put(WAIT_FOR_FINAL_STATE, requestBody.waitForFinalState);
+ remoteMessage.put(QUEUE_OPERATION, CollectionAction.REPLACENODE.toLower());
+
+ return new ZkNodeProps(remoteMessage);
+ }
+
+ public static class ReplaceNodeRequestBody implements
JacksonReflectMapWriter {
+
+ public ReplaceNodeRequestBody() {}
+
+ public ReplaceNodeRequestBody(String targetNodeName, Boolean
waitForFinalState) {
+ this.targetNodeName = targetNodeName;
+ this.waitForFinalState = waitForFinalState;
+ }
+
Review Comment:
[-1] v1 REPLACENODE supports the "async" parameter, so we should probably
add that to the RequestBody as well.
(`CollectionsHandler.submitCollectionApiCommand`, which you use above,
already has logic built around the parameter, so all we should have to do is
add it to the RequestBody here, and then make sure it gets added to the message
in `createRemoteMessage` above.)
##########
solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java:
##########
@@ -1791,14 +1794,17 @@ public Map<String, Object> execute(
REPLACENODE_OP(
REPLACENODE,
(req, rsp, h) -> {
- return copy(
- req.getParams(),
- null,
- "source", // legacy
Review Comment:
[-1] I'm fine with dropping the deprecated old parameter names in this PR,
but if we drop them here, we should also drop the code that checks for them in
[ReplaceNodeCmd](https://github.com/apache/solr/blob/c99af207c761ec34812ef1cc3054eb2804b7448b/solr/core/src/java/org/apache/solr/cloud/api/collections/ReplaceNodeCmd.java#L67)
##########
solr/core/src/java/org/apache/solr/handler/admin/api/ReplaceNodeAPI.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.handler.admin.api;
+
+import static
org.apache.solr.client.solrj.impl.BinaryResponseParser.BINARY_CONTENT_TYPE_V2;
+import static org.apache.solr.cloud.Overseer.QUEUE_OPERATION;
+import static org.apache.solr.common.params.CollectionParams.SOURCE_NODE;
+import static org.apache.solr.common.params.CollectionParams.TARGET_NODE;
+import static
org.apache.solr.common.params.CommonAdminParams.WAIT_FOR_FINAL_STATE;
+import static
org.apache.solr.handler.admin.CollectionsHandler.DEFAULT_COLLECTION_OP_TIMEOUT;
+import static
org.apache.solr.security.PermissionNameProvider.Name.COLL_EDIT_PERM;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import io.swagger.v3.oas.annotations.Parameter;
+import io.swagger.v3.oas.annotations.media.Schema;
+import io.swagger.v3.oas.annotations.parameters.RequestBody;
+import java.util.HashMap;
+import java.util.Map;
+import javax.inject.Inject;
+import javax.ws.rs.POST;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import org.apache.solr.client.solrj.SolrResponse;
+import org.apache.solr.common.cloud.ZkNodeProps;
+import org.apache.solr.common.params.CollectionParams;
+import org.apache.solr.common.params.CollectionParams.CollectionAction;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.handler.admin.CollectionsHandler;
+import org.apache.solr.jersey.JacksonReflectMapWriter;
+import org.apache.solr.jersey.PermissionName;
+import org.apache.solr.jersey.SolrJerseyResponse;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+
+/**
+ * V2 API for recreating replicas in one node (the source) on another node(s)
(the target).
+ *
+ * <p>This API is analogous to the v1 /admin/collections?action=REPLACENODE
command.
+ */
+@Path("cluster/nodes/{sourceNodeName}/commands/replace/")
+public class ReplaceNodeAPI extends AdminAPIBase {
+
+ @Inject
+ public ReplaceNodeAPI(
+ CoreContainer coreContainer,
+ SolrQueryRequest solrQueryRequest,
+ SolrQueryResponse solrQueryResponse) {
+ super(coreContainer, solrQueryRequest, solrQueryResponse);
+ }
+
+ @POST
+ @Produces({"application/json", "application/xml", BINARY_CONTENT_TYPE_V2})
+ @PermissionName(COLL_EDIT_PERM)
+ public SolrJerseyResponse replaceNode(
+ @Parameter(description = "The name of the node to be replaced.",
required = true)
+ @PathParam("sourceNodeName")
+ String nodeName,
+ @RequestBody(description = "Contains user provided parameters", required
= true)
+ ReplaceNodeRequestBody requestBody)
+ throws Exception {
+ final SolrJerseyResponse response =
instantiateJerseyResponse(SolrJerseyResponse.class);
+ final CoreContainer coreContainer =
fetchAndValidateZooKeeperAwareCoreContainer();
+ /** TODO Record node for log and tracing */
+ final ZkNodeProps remoteMessage = createRemoteMessage(nodeName,
requestBody);
+ final SolrResponse remoteResponse =
+ CollectionsHandler.submitCollectionApiCommand(
+ coreContainer,
+ coreContainer.getDistributedCollectionCommandRunner(),
+ remoteMessage,
+ CollectionParams.CollectionAction.REPLACENODE,
+ DEFAULT_COLLECTION_OP_TIMEOUT);
+ if (remoteResponse.getException() != null) {
+ throw remoteResponse.getException();
+ }
+
+ disableResponseCaching();
+ return response;
+ }
+
+ public ZkNodeProps createRemoteMessage(String nodeName,
ReplaceNodeRequestBody requestBody) {
+ final Map<String, Object> remoteMessage = new HashMap<>();
+ remoteMessage.put(SOURCE_NODE, nodeName);
+ remoteMessage.put(TARGET_NODE, requestBody.targetNodeName);
Review Comment:
[0] If the user doesn't provide a request-body, e.g.
```
curl -ilk -X POST
"http://localhost:8983/api/cluster/nodes/localhost:7574_solr/commands/replace";
```
then this line will cause a NullPointerException.
I'm a little torn on this - I think it'd technically be "correct" if we
forced users to provide an empty JSON object (`{}`) as the request body even if
they don't want to specify any of the optional parameters. But in practice -
idk, it seems like it'd be a pain that we could save our users.
What do you think of surrounding these lines with a `if (requestBody !=
null)` check?
Also, this would be a great this to have a test for later on when you get to
that point.
--
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]
