gerlowskija commented on code in PR #1126:
URL: https://github.com/apache/solr/pull/1126#discussion_r1107644285
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -1095,26 +1095,21 @@ protected void configure() {
.to(SolrNodeKeyPair.class)
.in(Singleton.class);
}
+ })
+ .register(
+ new AbstractBinder() {
+ @Override
+ protected void configure() {
+ bindFactory(
+ new InjectionFactories.SingletonFactory<>(
+ coreAdminHandler.getCoreAdminAsyncTracker()))
+ .to(CoreAdminHandler.CoreAdminAsyncTracker.class)
+ .in(Singleton.class);
+ }
});
jerseyAppHandler = new
ApplicationHandler(containerHandlers.getJerseyEndpoints());
}
- // Do Node setup logic after all handlers have been registered.
Review Comment:
[Q] Hmm...Do you know why this is deleted on your branch? Doesn't seem
related to this JIRA...
##########
solr/core/src/java/org/apache/solr/handler/admin/api/CoreSnapshotAPI.java:
##########
@@ -0,0 +1,280 @@
+/*
+ * 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.security.PermissionNameProvider.Name.CORE_EDIT_PERM;
+import static
org.apache.solr.security.PermissionNameProvider.Name.CORE_READ_PERM;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import io.swagger.v3.oas.annotations.Parameter;
+import io.swagger.v3.oas.annotations.media.Schema;
+import java.io.IOException;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Optional;
+import javax.inject.Inject;
+import javax.ws.rs.DELETE;
+import javax.ws.rs.GET;
+import javax.ws.rs.POST;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import org.apache.lucene.index.IndexCommit;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.params.CoreAdminParams;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.core.IndexDeletionPolicyWrapper;
+import org.apache.solr.core.SolrCore;
+import org.apache.solr.core.snapshots.SolrSnapshotManager;
+import org.apache.solr.core.snapshots.SolrSnapshotMetaDataManager;
+import org.apache.solr.handler.admin.CoreAdminHandler;
+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 Creating, Listing, and Deleting Core Snapshots. */
+@Path("/cores/{coreName}/snapshots")
+public class CoreSnapshotAPI extends CoreAdminAPIBase {
+
+ @Inject
+ public CoreSnapshotAPI(
+ SolrQueryRequest request,
+ SolrQueryResponse response,
+ CoreContainer coreContainer,
+ CoreAdminHandler.CoreAdminAsyncTracker coreAdminAsyncTracker) {
+ super(coreContainer, coreAdminAsyncTracker, request, response);
+ }
+
+ /** This API is analogous to V1 (POST
/solr/admin/cores?action=CREATESNAPSHOT) */
+ @POST
+ @Path("/{snapshotName}")
+ @Produces({"application/json", "application/xml", BINARY_CONTENT_TYPE_V2})
+ @PermissionName(CORE_EDIT_PERM)
+ public CreateSnapshotResponse createSnapshot(
+ @Parameter(description = "The name of the core to snapshot.", required =
true)
+ @PathParam("coreName")
+ String coreName,
+ @Parameter(description = "The name to associate with the core
snapshot.", required = true)
+ @PathParam("snapshotName")
+ String snapshotName,
+ @Parameter(description = "The id to associate with the async task.")
@QueryParam("async")
+ String taskId)
+ throws Exception {
+ final CreateSnapshotResponse response =
instantiateJerseyResponse(CreateSnapshotResponse.class);
+
+ return handle(
+ response,
+ coreName,
+ taskId,
+ "createSnapshot",
+ () -> {
+ try (SolrCore core = coreContainer.getCore(coreName)) {
+ if (core == null) {
+ throw new SolrException(
+ SolrException.ErrorCode.BAD_REQUEST, "Unable to locate core
" + coreName);
+ }
+
+ final String indexDirPath = core.getIndexDir();
+ final IndexDeletionPolicyWrapper delPol = core.getDeletionPolicy();
+ final IndexCommit ic = delPol.getAndSaveLatestCommit();
+ try {
+ if (null == ic) {
+ throw new SolrException(
+ SolrException.ErrorCode.BAD_REQUEST,
+ "No index commits to snapshot in core " + coreName);
+ }
+ final SolrSnapshotMetaDataManager mgr =
core.getSnapshotMetaDataManager();
+ mgr.snapshot(snapshotName, indexDirPath, ic.getGeneration());
+
+ response.core = core.getName();
+ response.commitName = snapshotName;
+ response.indexDirPath = indexDirPath;
+ response.generation = ic.getGeneration();
+ response.files = ic.getFileNames();
+ } catch (IOException e) {
+ throw new CoreAdminAPIBaseException(e);
+ } finally {
+ delPol.releaseCommitPoint(ic);
+ }
+ }
+
+ return response;
+ });
+ }
+
+ /** The Response for {@link CoreSnapshotAPI}'s {@link
#createSnapshot(String, String, String)} */
+ public static class CreateSnapshotResponse extends SolrJerseyResponse {
+ @Schema(description = "The name of the core.")
Review Comment:
[+1] Awesome, thanks for the OpenAPI docs on these fields!
##########
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java:
##########
@@ -493,4 +434,94 @@ public interface CoreAdminOp {
*/
void execute(CallInfo it) throws Exception;
}
+
+ public static class CoreAdminAsyncTracker {
+ private static final int MAX_TRACKED_REQUESTS = 100;
+ public static final String RUNNING = "running";
+ public static final String COMPLETED = "completed";
+ public static final String FAILED = "failed";
+ public final Map<String, Map<String, TaskObject>> requestStatusMap;
+
+ private ExecutorService parallelExecutor =
+ ExecutorUtil.newMDCAwareFixedThreadPool(
+ 50, new
SolrNamedThreadFactory("parallelCoreAdminAPIBaseExecutor"));
+
+ public CoreAdminAsyncTracker() {
+ HashMap<String, Map<String, TaskObject>> map = new HashMap<>(3, 1.0f);
+ map.put(RUNNING, Collections.synchronizedMap(new LinkedHashMap<>()));
+ map.put(COMPLETED, Collections.synchronizedMap(new LinkedHashMap<>()));
+ map.put(FAILED, Collections.synchronizedMap(new LinkedHashMap<>()));
+ requestStatusMap = Collections.unmodifiableMap(map);
+ }
+
+ public void shutdown() {
+ ExecutorUtil.shutdownAndAwaitTermination(parallelExecutor);
+ }
+
+ public Map<String, TaskObject> getRequestStatusMap(String key) {
+ return requestStatusMap.get(key);
+ }
+
+ public ExecutorService getParallelExecutor() {
+ return parallelExecutor;
+ }
+
+ /** Helper method to add a task to a tracking type. */
+ public void addTask(String type, TaskObject o, boolean limit) {
+ synchronized (getRequestStatusMap(type)) {
+ if (limit && getRequestStatusMap(type).size() == MAX_TRACKED_REQUESTS)
{
+ String key =
getRequestStatusMap(type).entrySet().iterator().next().getKey();
+ getRequestStatusMap(type).remove(key);
+ }
+ addTask(type, o);
+ }
+ }
+
+ public void addTask(String type, TaskObject o) {
+ synchronized (getRequestStatusMap(type)) {
+ getRequestStatusMap(type).put(o.taskId, o);
+ }
+ }
+
+ /** Helper method to remove a task from a tracking map. */
+ public void removeTask(String map, String taskId) {
+ synchronized (getRequestStatusMap(map)) {
Review Comment:
Let's ignore this 'lift' warning for now - it's coming from code that has
only been moved around (and not changed) as a part of this PR.
Might be worth adding a TODO comment somewhere in this code though that
proposes making the status options (RUNNING, COMPLETED, FAILED) into an enum
though, to provide a little more type-safety going forward.
##########
solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc:
##########
@@ -308,37 +330,45 @@ The name of the core to whose snapshots we want to list.
+
Request ID to track this action which will be processed asynchronously.
-=== Delete Snapshot API
+=== LIST Response
+
+The response will include the status of the request and all existing snapshots
for the core.
+If the status is anything other than "success", an error message will explain
why the request failed.
-The `DELETESNAPSHOT` command deletes a snapshot for a particular core.
+[[delete-snapshot-api]]
+== DELETE: Delete a Snapshot
You can trigger a delete snapshot with an HTTP command like this (replace
"techproducts" with the name of the core you are working with):
-.Delete Snapshot API Example
-[source,text]
+[.dynamic-tabs]
+--
+[example.tab-pane#v1deletesnapshot]
+====
+[.tab-label]*V1 API*
+
+[source,bash]
----
-http://localhost:8983/solr/admin/cores?action=DELETESNAPSHOT&core=techproducts&commitName=commit1
+curl -X DELETE
http://localhost:8983/solr/admin/cores?action=DELETESNAPSHOT&core=techproducts&commitName=commit1
Review Comment:
[-0] We should probably drop the `-X DELETE` in the v1 example, it's never
been documented that way before and I'd be surprised if anything other than a
GET or POST worked for v1.
##########
solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc:
##########
@@ -276,28 +281,45 @@ The name of the core to perform the snapshot on.
+
Request ID to track this action which will be processed asynchronously.
-=== List Snapshot API
+=== CREATE Response
+
+The response will include the status of the request, the core name, snapshot
name, index directory path, snapshot generation, and a list of file names.
+If the status is anything other than "success", an error message will explain
why the request failed.
-The `LISTSNAPSHOTS` command lists all the taken snapshots for a particular
core.
+[[list-snapshot-api]]
+== List: List All Snapshots for a Particular Core
You can trigger a list snapshot command with an HTTP command like this
(replace "techproducts" with the name of the core you are working with):
-.List Snapshot API
-[source,text]
+[.dynamic-tabs]
+--
+[example.tab-pane#v1listsnapshots]
+====
+[.tab-label]*V1 API*
+
+[source,bash]
----
-http://localhost:8983/solr/admin/cores?action=LISTSNAPSHOTS&core=techproducts&commitName=commit1
+curl
http://localhost:8983/solr/admin/cores?action=LISTSNAPSHOTS&core=techproducts&commitName=commit1
+
----
+====
-The list snapshot request parameters are:
+[example.tab-pane#v2listsnapshots]
+====
+[.tab-label]*V2 API*
-`core`::
-+
-[%autowidth,frame=none]
-|===
-|Optional |Default: none
-|===
-+
-The name of the core to whose snapshots we want to list.
+With the v2 API, the core and snapshot names are part of the path instead of
query parameters.
Review Comment:
[-0] Small nit: this looks like a copy/paste oversight - there is no
snapshot name in the LIST API.
##########
solr/core/src/java/org/apache/solr/handler/admin/api/CoreAdminAPIBase.java:
##########
@@ -0,0 +1,157 @@
+/*
+ * 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.handler.admin.CoreAdminHandler.CoreAdminAsyncTracker.COMPLETED;
+import static
org.apache.solr.handler.admin.CoreAdminHandler.CoreAdminAsyncTracker.FAILED;
+import static
org.apache.solr.handler.admin.CoreAdminHandler.CoreAdminAsyncTracker.RUNNING;
+
+import java.util.function.Supplier;
+import org.apache.solr.api.JerseyResource;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.handler.admin.CoreAdminHandler;
+import org.apache.solr.handler.api.V2ApiUtils;
+import org.apache.solr.jersey.SolrJerseyResponse;
+import org.apache.solr.logging.MDCLoggingContext;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.util.tracing.TraceUtils;
+import org.slf4j.MDC;
+
+/**
+ * A common parent for admin Core Jersey-based APIs.
+ *
+ * <p>This base class is used when creating Core APIs to allow extra
bookkeeping tasks such as async
+ * requests handling.
+ */
+public abstract class CoreAdminAPIBase extends JerseyResource {
+
+ protected final CoreContainer coreContainer;
+ protected final CoreAdminHandler.CoreAdminAsyncTracker coreAdminAsyncTracker;
+ protected final SolrQueryRequest req;
+ protected final SolrQueryResponse rsp;
+
+ public CoreAdminAPIBase(
+ CoreContainer coreContainer,
+ CoreAdminHandler.CoreAdminAsyncTracker coreAdminAsyncTracker,
+ SolrQueryRequest req,
+ SolrQueryResponse rsp) {
+ this.coreContainer = coreContainer;
+ this.coreAdminAsyncTracker = coreAdminAsyncTracker;
+ this.req = req;
+ this.rsp = rsp;
+ }
+
+ /**
+ * Wraps the subclasses logic with extra bookkeeping logic.
+ *
+ * <p>This method currently exists to enable async handling behavior for V2
Core APIs.
+ *
+ * <p>Since the logic for a given API lives inside the Supplier functional
interface, checked
+ * exceptions can't be thrown directly to the calling method. To throw a
checked exception out of
+ * the Supplier, wrap the exception using {@link
CoreAdminAPIBase.CoreAdminAPIBaseException} and
+ * throw it instead. This handle method will retrieve the checked exception
from {@link
+ * CoreAdminAPIBaseException} and throw it to the original calling method.
+ *
+ * @param solrJerseyResponse the response that the calling methods expects
to return.
+ * @param coreName the name of the core that work is being done against.
+ * @param taskId an id provided for registering async work.
+ * @param actionName a name for the action being done.
+ * @param supplier the work that the calling method wants done.
+ * @return the supplied T solrJerseyResponse
+ */
+ public <T extends SolrJerseyResponse> T handle(
Review Comment:
[+1] Overall I really like how this approach panned out; very cool!
[0] Small nit: can you think of a name here that'd make the method's purpose
clearer than `handle`? I'm struggling a bit, but 'handle' feels generic. Maybe
`handlePotentiallyAsynchronousTask`?
[-0] The one downside of this approach is that CoreAdminHandler still has a
lot of code that looks similar to what's in this method. (Specifically lines
200-250ish in CoreAdminHandler.) Maybe this is as good as it gets, but can you
think of any way we could refactor CoreAdminHandler, or this method, or both to
cut down on the duplication between the two?
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
