epugh commented on code in PR #4452:
URL: https://github.com/apache/solr/pull/4452#discussion_r3320920762
##########
solr/api/src/java/org/apache/solr/client/api/endpoint/TasksApi.java:
##########
@@ -27,23 +27,26 @@
import org.apache.solr.client.api.model.TaskStatusResponse;
import org.apache.solr.client.api.util.StoreApiParameters;
-@Path(INDEX_PATH_PREFIX + "/tasks/list")
-public interface ListActiveTasksApi {
+@Path(INDEX_PATH_PREFIX)
+public interface TasksApi {
- // Handles: .../tasks/list (Lists all)
- @GET
- @StoreApiParameters
- @Operation(
- summary = "Lists all the currently running tasks",
- tags = {"tasks"})
- ListActiveTaskResponse listAllActiveTasks() throws Exception;
+ @Path("/tasks")
+ interface List {
+ @GET
+ @StoreApiParameters
+ @Operation(
+ summary = "Lists all the currently running active tasks.",
+ tags = {"tasks"})
+ ListActiveTaskResponse listAllActiveTasks() throws Exception;
+ }
- // Handles: .../tasks/list/slow-task-id (Lists specific)
- @GET
- @Path("/{taskUUID}")
- @StoreApiParameters
- @Operation(
- summary = "Status of a specific taskUUID passed as pathParam",
- tags = {"tasks"})
- TaskStatusResponse getTaskStatus(@PathParam("taskUUID") String taskUUID)
throws Exception;
+ @Path("/tasks/{taskID}")
+ interface Status {
+ @GET
+ @StoreApiParameters
+ @Operation(
+ summary = "Status of a specific taskID passed as pathParam.",
+ tags = {"tasks"})
+ TaskStatusResponse getTaskStatus(@PathParam("taskID") String taskID)
throws Exception;
Review Comment:
We should remember to rationalize all our various "id" related attributes to
have a similar pattern... across all the v2 apis... ;-)
##########
solr/api/src/java/org/apache/solr/client/api/endpoint/TasksApi.java:
##########
@@ -27,23 +27,26 @@
import org.apache.solr.client.api.model.TaskStatusResponse;
import org.apache.solr.client.api.util.StoreApiParameters;
-@Path(INDEX_PATH_PREFIX + "/tasks/list")
-public interface ListActiveTasksApi {
+@Path(INDEX_PATH_PREFIX)
+public interface TasksApi {
Review Comment:
nice!
##########
solr/core/src/java/org/apache/solr/handler/admin/api/GetTaskStatus.java:
##########
@@ -0,0 +1,51 @@
+/*
+ * 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.security.PermissionNameProvider.Name.READ_PERM;
+
+import org.apache.solr.api.JerseyResource;
+import org.apache.solr.client.api.endpoint.TasksApi;
+import org.apache.solr.client.api.model.TaskStatusResponse;
+import org.apache.solr.jersey.PermissionName;
+import org.apache.solr.request.SolrQueryRequest;
+
+public class GetTaskStatus extends JerseyResource implements TasksApi.Status {
+
+ private final SolrQueryRequest solrQueryRequest;
+
+ public GetTaskStatus(SolrQueryRequest solrQueryRequest) {
+ this.solrQueryRequest = solrQueryRequest;
+ }
+
+ @Override
+ @PermissionName(READ_PERM)
+ public TaskStatusResponse getTaskStatus(String taskID) throws Exception {
+ final TaskStatusResponse response =
instantiateJerseyResponse(TaskStatusResponse.class);
+
+ boolean isTaskActive =
+
solrQueryRequest.getCore().getCancellableQueryTracker().isQueryIdActive(taskID);
+
+ response.taskStatus =
+ (isTaskActive)
+ ? TaskStatusResponse.TaskStatus.ACTIVE
Review Comment:
I think I would just import `TaskStatusResponse` to have a less nested
strcutrure.
##########
solr/core/src/java/org/apache/solr/handler/component/ActiveTasksListHandler.java:
##########
@@ -23,37 +23,42 @@
import org.apache.solr.api.Api;
import org.apache.solr.api.JerseyResource;
import org.apache.solr.client.api.model.ActiveTaskDetails;
+import org.apache.solr.client.api.model.TaskStatusResponse;
import org.apache.solr.common.util.NamedList;
+import org.apache.solr.handler.admin.api.GetTaskStatus;
import org.apache.solr.handler.admin.api.ListActiveTasks;
-import org.apache.solr.handler.api.V2ApiUtils;
import org.apache.solr.request.SolrQueryRequest;
import org.apache.solr.request.SolrRequestHandler;
import org.apache.solr.response.SolrQueryResponse;
import org.apache.solr.security.AuthorizationContext;
import org.apache.solr.security.PermissionNameProvider;
/**
- * Handles request for listing all active cancellable tasks All active tasks
logic lives in the v2
- * {@link ListActiveTasks}; this handler is a thin v1 bridge that extracts
request parameters and
- * delegates.
+ * Handles request for listing the active cancellable tasks and Status check
of any particular task,
+ * actual logic lives in the v2: {@link ListActiveTasks} and {@link
GetTaskStatus}; this handler is
+ * a thin v1 bridge that extracts request parameters and delegates over to v2.
*/
public class ActiveTasksListHandler extends TaskManagementHandler {
// This can be a parent level member but we keep it here to allow future
handlers to have
// a custom list of components
@Override
public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp)
throws Exception {
- String taskStatusCheckUUID = req.getParams().get(TASK_CHECK_UUID, null);
+ String taskStatusCheckID = req.getParams().get(TASK_CHECK_UUID, null);
Review Comment:
so, we can't change any signatures of v1, so I don't know if
`TASK_CHECK_UUID` changed? I think it's fine to leave "taskStatusCheckUUID",
as we hope to get rid of it? Maybe just a comment? I dunno.
Or, we change `taskStatusCheckID` to just `taskID` internally if we are
going to rename it?
##########
solr/core/src/java/org/apache/solr/handler/admin/api/GetTaskStatus.java:
##########
@@ -0,0 +1,51 @@
+/*
+ * 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.security.PermissionNameProvider.Name.READ_PERM;
+
+import org.apache.solr.api.JerseyResource;
+import org.apache.solr.client.api.endpoint.TasksApi;
+import org.apache.solr.client.api.model.TaskStatusResponse;
+import org.apache.solr.jersey.PermissionName;
+import org.apache.solr.request.SolrQueryRequest;
+
+public class GetTaskStatus extends JerseyResource implements TasksApi.Status {
+
+ private final SolrQueryRequest solrQueryRequest;
+
+ public GetTaskStatus(SolrQueryRequest solrQueryRequest) {
+ this.solrQueryRequest = solrQueryRequest;
+ }
+
+ @Override
+ @PermissionName(READ_PERM)
+ public TaskStatusResponse getTaskStatus(String taskID) throws Exception {
+ final TaskStatusResponse response =
instantiateJerseyResponse(TaskStatusResponse.class);
+
+ boolean isTaskActive =
+
solrQueryRequest.getCore().getCancellableQueryTracker().isQueryIdActive(taskID);
+
+ response.taskStatus =
+ (isTaskActive)
+ ? TaskStatusResponse.TaskStatus.ACTIVE
Review Comment:
actually, we have that, so can we just refer to `TaskSTatus`?
##########
solr/api/src/java/org/apache/solr/client/api/endpoint/TasksApi.java:
##########
@@ -27,23 +27,26 @@
import org.apache.solr.client.api.model.TaskStatusResponse;
import org.apache.solr.client.api.util.StoreApiParameters;
-@Path(INDEX_PATH_PREFIX + "/tasks/list")
-public interface ListActiveTasksApi {
+@Path(INDEX_PATH_PREFIX)
+public interface TasksApi {
- // Handles: .../tasks/list (Lists all)
- @GET
- @StoreApiParameters
- @Operation(
- summary = "Lists all the currently running tasks",
- tags = {"tasks"})
- ListActiveTaskResponse listAllActiveTasks() throws Exception;
+ @Path("/tasks")
+ interface List {
+ @GET
+ @StoreApiParameters
+ @Operation(
+ summary = "Lists all the currently running active tasks.",
+ tags = {"tasks"})
+ ListActiveTaskResponse listAllActiveTasks() throws Exception;
+ }
- // Handles: .../tasks/list/slow-task-id (Lists specific)
- @GET
- @Path("/{taskUUID}")
- @StoreApiParameters
- @Operation(
- summary = "Status of a specific taskUUID passed as pathParam",
- tags = {"tasks"})
- TaskStatusResponse getTaskStatus(@PathParam("taskUUID") String taskUUID)
throws Exception;
+ @Path("/tasks/{taskID}")
+ interface Status {
+ @GET
+ @StoreApiParameters
+ @Operation(
+ summary = "Status of a specific taskID passed as pathParam.",
Review Comment:
I don't think we need the `passed as pathParam`, that is obvious from thow
the docs are generated.. In fact, I would do `Status of a specific task.`
and leave it at that.
##########
solr/api/src/java/org/apache/solr/client/api/endpoint/TasksApi.java:
##########
@@ -27,23 +27,26 @@
import org.apache.solr.client.api.model.TaskStatusResponse;
import org.apache.solr.client.api.util.StoreApiParameters;
-@Path(INDEX_PATH_PREFIX + "/tasks/list")
-public interface ListActiveTasksApi {
+@Path(INDEX_PATH_PREFIX)
+public interface TasksApi {
- // Handles: .../tasks/list (Lists all)
- @GET
- @StoreApiParameters
- @Operation(
- summary = "Lists all the currently running tasks",
- tags = {"tasks"})
- ListActiveTaskResponse listAllActiveTasks() throws Exception;
+ @Path("/tasks")
+ interface List {
+ @GET
+ @StoreApiParameters
+ @Operation(
+ summary = "Lists all the currently running active tasks.",
Review Comment:
I wonder if this should be `Lists all the currently active tasks.`.
Running/not running, that really isn't an aspect. The task is either active or
not, at least, that is how I read the `TaskStatus` enum! Nit picky, but now
is the time.
##########
solr/core/src/java/org/apache/solr/handler/component/ActiveTasksListHandler.java:
##########
@@ -23,37 +23,42 @@
import org.apache.solr.api.Api;
import org.apache.solr.api.JerseyResource;
import org.apache.solr.client.api.model.ActiveTaskDetails;
+import org.apache.solr.client.api.model.TaskStatusResponse;
import org.apache.solr.common.util.NamedList;
+import org.apache.solr.handler.admin.api.GetTaskStatus;
import org.apache.solr.handler.admin.api.ListActiveTasks;
-import org.apache.solr.handler.api.V2ApiUtils;
import org.apache.solr.request.SolrQueryRequest;
import org.apache.solr.request.SolrRequestHandler;
import org.apache.solr.response.SolrQueryResponse;
import org.apache.solr.security.AuthorizationContext;
import org.apache.solr.security.PermissionNameProvider;
/**
- * Handles request for listing all active cancellable tasks All active tasks
logic lives in the v2
- * {@link ListActiveTasks}; this handler is a thin v1 bridge that extracts
request parameters and
- * delegates.
+ * Handles request for listing the active cancellable tasks and Status check
of any particular task,
Review Comment:
also, maybe a typo of "Status" check? Lastly, this whole sentence maybe
needs a period? Just appears a bit awkared.
In fact, i think we don't need the text about "actual logic", i don't think
we do that in other apis? If we do, then great...
##########
solr/core/src/java/org/apache/solr/handler/admin/api/ListActiveTasks.java:
##########
@@ -25,14 +25,13 @@
import java.util.List;
import java.util.Map;
import org.apache.solr.api.JerseyResource;
-import org.apache.solr.client.api.endpoint.ListActiveTasksApi;
+import org.apache.solr.client.api.endpoint.TasksApi;
import org.apache.solr.client.api.model.ActiveTaskDetails;
import org.apache.solr.client.api.model.ListActiveTaskResponse;
-import org.apache.solr.client.api.model.TaskStatusResponse;
import org.apache.solr.jersey.PermissionName;
import org.apache.solr.request.SolrQueryRequest;
-public class ListActiveTasks extends JerseyResource implements
ListActiveTasksApi {
+public class ListActiveTasks extends JerseyResource implements TasksApi.List {
Review Comment:
this reads nicer.
##########
solr/core/src/java/org/apache/solr/handler/component/ActiveTasksListHandler.java:
##########
@@ -23,37 +23,42 @@
import org.apache.solr.api.Api;
import org.apache.solr.api.JerseyResource;
import org.apache.solr.client.api.model.ActiveTaskDetails;
+import org.apache.solr.client.api.model.TaskStatusResponse;
import org.apache.solr.common.util.NamedList;
+import org.apache.solr.handler.admin.api.GetTaskStatus;
import org.apache.solr.handler.admin.api.ListActiveTasks;
-import org.apache.solr.handler.api.V2ApiUtils;
import org.apache.solr.request.SolrQueryRequest;
import org.apache.solr.request.SolrRequestHandler;
import org.apache.solr.response.SolrQueryResponse;
import org.apache.solr.security.AuthorizationContext;
import org.apache.solr.security.PermissionNameProvider;
/**
- * Handles request for listing all active cancellable tasks All active tasks
logic lives in the v2
- * {@link ListActiveTasks}; this handler is a thin v1 bridge that extracts
request parameters and
- * delegates.
+ * Handles request for listing the active cancellable tasks and Status check
of any particular task,
Review Comment:
are there active tasks that are NOT cancellable?
--
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]
