[GitHub] [solr] gerlowskija commented on a diff in pull request #1449: SOLR-16490 Create a v2 equivalent for RESTORECORE

["gerlowskija (via GitHub)" <gi...@apache.org>]

gerlowskija commented on code in PR #1449:
URL: https://github.com/apache/solr/pull/1449#discussion_r1156337270


##########
solr/core/src/java/org/apache/solr/handler/admin/api/RestoreCoreAPI.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.SolrRequest.METHOD.POST;
+import static org.apache.solr.common.params.CoreAdminParams.ACTION;
+import static org.apache.solr.common.params.CoreAdminParams.CORE;
+import static org.apache.solr.common.params.CoreAdminParams.CoreAdminAction.RESTORECORE;
+import static org.apache.solr.handler.ClusterAPI.wrapParams;
+import static org.apache.solr.security.PermissionNameProvider.Name.CORE_EDIT_PERM;
+
+import java.util.HashMap;
+import java.util.Locale;
+import java.util.Map;
+import org.apache.solr.api.Command;
+import org.apache.solr.api.EndPoint;
+import org.apache.solr.api.PayloadObj;
+import org.apache.solr.client.solrj.request.beans.RestoreCorePayload;
+import org.apache.solr.handler.admin.CoreAdminHandler;
+
+/**
+ * Internal V2 API triggering restore a core.
+ *
+ * <p>Only valid in SolrCloud mode. This API (POST /api/cores/coreName/restore {'restore': {}}) is
+ * analogous to the v1 GET /solr/admin/cores?action=RESTORECORE command.
+ *
+ * @see RestoreCorePayload
+ */
+@EndPoint(
+    path = {"/cores/{core}"},

Review Comment:
   [-1] IMO the endpoint specifics (particularly the path/url) should look a little different here.
   
   It might've gotten buried in all the text on SOLR-16490, but we've been building up [a spreadsheet](https://docs.google.com/spreadsheets/d/1HAoBBFPpSiT8mJmgNZKkZAPwfCfPvlc08m5jz3fQBpA) of Solr's APIs and what their v2 equivalents will ideally look like.  RESTORECORE is a bit hard to find there, but on the "'Command' APIs" Tab there's an entry for it.  The path there is listed as `/cores/{core}/restore`- could we use that value here?



##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/beans/RestoreCorePayload.java:
##########
@@ -0,0 +1,22 @@
+/*
+ * 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.client.solrj.request.beans;
+
+import org.apache.solr.common.util.ReflectMapWriter;
+
+public class RestoreCorePayload implements ReflectMapWriter {}

Review Comment:
   [0] RestoreCore operations supports a bunch of params that we'll need to include in the payload/request-body here.  For a sense of what all 'restore' supports, see the `req.getParams()` calls in `RestoreCoreOp`



##########
solr/core/src/java/org/apache/solr/handler/admin/api/RestoreCoreAPI.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.SolrRequest.METHOD.POST;
+import static org.apache.solr.common.params.CoreAdminParams.ACTION;
+import static org.apache.solr.common.params.CoreAdminParams.CORE;
+import static org.apache.solr.common.params.CoreAdminParams.CoreAdminAction.RESTORECORE;
+import static org.apache.solr.handler.ClusterAPI.wrapParams;
+import static org.apache.solr.security.PermissionNameProvider.Name.CORE_EDIT_PERM;
+
+import java.util.HashMap;
+import java.util.Locale;
+import java.util.Map;
+import org.apache.solr.api.Command;
+import org.apache.solr.api.EndPoint;
+import org.apache.solr.api.PayloadObj;
+import org.apache.solr.client.solrj.request.beans.RestoreCorePayload;
+import org.apache.solr.handler.admin.CoreAdminHandler;
+
+/**
+ * Internal V2 API triggering restore a core.
+ *
+ * <p>Only valid in SolrCloud mode. This API (POST /api/cores/coreName/restore {'restore': {}}) is
+ * analogous to the v1 GET /solr/admin/cores?action=RESTORECORE command.
+ *
+ * @see RestoreCorePayload
+ */
+@EndPoint(
+    path = {"/cores/{core}"},
+    method = POST,
+    permission = CORE_EDIT_PERM)
+public class RestoreCoreAPI {
+
+  public static final String V2_CORE_RESTORE_CMD = "restore";
+
+  private final CoreAdminHandler coreAdminHandler;
+
+  public RestoreCoreAPI(CoreAdminHandler coreAdminHandler) {
+    this.coreAdminHandler = coreAdminHandler;
+  }
+
+  @Command(name = V2_CORE_RESTORE_CMD)

Review Comment:
   [-1] Similar my comment above about the API path, I think the overall shape of this code looks great.  Just need to tweak a few specifics so that the v2 RESTORECORE API lines up better with the direction we're trying to move our v2 APIs in.
   
   Particularly, we're trying to move away from the v2 "command-based" APIs (i.e. APIs with a request body whose only top-level key is the name of some command or other).  Instead we're generally moving the command itself into the path (see my comment above about changing the path to `/cores/coreName/restore`) and then moving most of the actual restore parameters to the top level of the "Payload"/request-body.
   
   This is definitely do-able using the Solr-custom annotation framework you've used in this PR (see RenameCollectionAPI for an example).  But you might find it more natural to do if you switch this API over to use the JAX-RS framework we've been moving towards.  If you want to try that out there's a step-by-step writeup on this [here](https://issues.apache.org/jira/browse/SOLR-15737?focusedCommentId=17617923&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17617923) and a handful of examples that you can crib from ([example1](https://github.com/apache/solr/pull/1452), [example2](https://github.com/apache/solr/pull/1441))



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