epugh commented on code in PR #4452: URL: https://github.com/apache/solr/pull/4452#discussion_r3314008365
########## solr/api/src/java/org/apache/solr/client/api/endpoint/ListActiveTasksApi.java: ########## @@ -0,0 +1,49 @@ +/* + * 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.api.endpoint; + +import static org.apache.solr.client.api.util.Constants.INDEX_PATH_PREFIX; + +import io.swagger.v3.oas.annotations.Operation; +import jakarta.ws.rs.GET; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.PathParam; +import org.apache.solr.client.api.model.ListActiveTaskResponse; +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 { Review Comment: I am looking at the `org.apache.solr.client.api.endpoint.ConfigSetsApi` and liking it's pattern a bit better. I'll try and provide some detailed feedback! I think there is an opportunity to have line 30 be `/tasks` ########## solr/api/src/java/org/apache/solr/client/api/endpoint/ListActiveTasksApi.java: ########## @@ -0,0 +1,49 @@ +/* + * 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.api.endpoint; + +import static org.apache.solr.client.api.util.Constants.INDEX_PATH_PREFIX; + +import io.swagger.v3.oas.annotations.Operation; +import jakarta.ws.rs.GET; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.PathParam; +import org.apache.solr.client.api.model.ListActiveTaskResponse; +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 { + + // Handles: .../tasks/list (Lists all) Review Comment: i don't think we need this comment. ########## solr/api/src/java/org/apache/solr/client/api/endpoint/ListActiveTasksApi.java: ########## @@ -0,0 +1,49 @@ +/* + * 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.api.endpoint; + +import static org.apache.solr.client.api.util.Constants.INDEX_PATH_PREFIX; + +import io.swagger.v3.oas.annotations.Operation; +import jakarta.ws.rs.GET; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.PathParam; +import org.apache.solr.client.api.model.ListActiveTaskResponse; +import org.apache.solr.client.api.model.TaskStatusResponse; +import org.apache.solr.client.api.util.StoreApiParameters; + +@Path(INDEX_PATH_PREFIX + "/tasks/list") Review Comment: if the path here is `/tasks` then we don't need the `/list`, as the pattern is GET `/tasks` will list all the tasks. This is more restful. ########## solr/api/src/java/org/apache/solr/client/api/endpoint/ListActiveTasksApi.java: ########## @@ -0,0 +1,49 @@ +/* + * 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.api.endpoint; + +import static org.apache.solr.client.api.util.Constants.INDEX_PATH_PREFIX; + +import io.swagger.v3.oas.annotations.Operation; +import jakarta.ws.rs.GET; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.PathParam; +import org.apache.solr.client.api.model.ListActiveTaskResponse; +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 { Review Comment: Maybe this is `TasksApi` ? ########## solr/api/src/java/org/apache/solr/client/api/endpoint/ListActiveTasksApi.java: ########## @@ -0,0 +1,49 @@ +/* + * 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.api.endpoint; + +import static org.apache.solr.client.api.util.Constants.INDEX_PATH_PREFIX; + +import io.swagger.v3.oas.annotations.Operation; +import jakarta.ws.rs.GET; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.PathParam; +import org.apache.solr.client.api.model.ListActiveTaskResponse; +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 { + + // Handles: .../tasks/list (Lists all) + @GET + @StoreApiParameters + @Operation( + summary = "Lists all the currently running tasks", + tags = {"tasks"}) + ListActiveTaskResponse listAllActiveTasks() throws Exception; + + // Handles: .../tasks/list/slow-task-id (Lists specific) + @GET + @Path("/{taskUUID}") Review Comment: I think the fact that the task identifier is a UUID isn't really important. I with this was more like `@Path("/tasks/{taskId}")` ########## solr/api/src/java/org/apache/solr/client/api/endpoint/ListActiveTasksApi.java: ########## Review Comment: I am expecting a `DELETE /tasks/{taskId}` for cancelling a running task. Or... we need a PATCH that does the cancel `PATCH /tasks/{taskId}/cancel` ########## solr/api/src/java/org/apache/solr/client/api/model/ActiveTaskDetails.java: ########## @@ -0,0 +1,33 @@ +/* + * 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.api.model; + +import com.fasterxml.jackson.annotation.JsonProperty; + +public class ActiveTaskDetails { + + public ActiveTaskDetails() {} + + public ActiveTaskDetails(String taskUUID, String taskQuery) { + this.taskUUID = taskUUID; Review Comment: you know, maybe we just keep `taskUUID` for now... just to reduce churn. Even though I don't like it as a name. ########## solr/core/src/test/org/apache/solr/search/TestTaskManagement.java: ########## @@ -213,6 +213,46 @@ public void testCheckSpecificQueryStatus() throws Exception { assertTrue(result.contains("inactive")); } + @Test + public void testCheckSpecificQueryStatus_Active() throws Exception { + for (int i = 0; i < 10; i++) { + executeQueryAsync(Integer.toString(i)); + } + + ModifiableSolrParams params = new ModifiableSolrParams(); + params.set("taskUUID", "5"); + + var request = + new GenericSolrRequest( + SolrRequest.METHOD.GET, "/tasks/list", SolrRequest.SolrRequestType.ADMIN, params) + .setRequiresCollection(true); + NamedList<Object> queryResponse = cluster.getSolrClient(COLLECTION_NAME).request(request); + + String result = (String) queryResponse.get("taskStatus"); + + assertTrue(result.contains("active")); + } + + @Test + public void testCheckSpecificQueryStatus_Inactive() throws Exception { + for (int i = 0; i < 10; i++) { Review Comment: why do we need to execute 10 of these? Seems like a magic number! ########## solr/api/src/java/org/apache/solr/client/api/model/TaskStatusResponse.java: ########## @@ -0,0 +1,24 @@ +/* + * 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.api.model; + +import com.fasterxml.jackson.annotation.JsonProperty; + +public class TaskStatusResponse extends SolrJerseyResponse { + @JsonProperty public String taskStatus; Review Comment: also, I am getting the feeling that taskStatus isnt' actually a good format as currently defined. I looked int he docs: ```{ "responseHeader":{ "status":0, "QTime":16}, "taskStatus":"id:5, status: active"}``` and at a minimum, it should just be: ``` { "responseHeader":{ "status":0, "QTime":16}, "taskStatus":"active"} ``` because we already know the id. Now, if we are listing, we should have something like: ``` { "responseHeader":{ "status":0, "QTime":16}, "tasks":[ "taskId":"5", "status":"active"} ] } ``` ########## solr/core/src/test/org/apache/solr/search/TestTaskManagement.java: ########## @@ -213,6 +213,46 @@ public void testCheckSpecificQueryStatus() throws Exception { assertTrue(result.contains("inactive")); } + @Test + public void testCheckSpecificQueryStatus_Active() throws Exception { + for (int i = 0; i < 10; i++) { + executeQueryAsync(Integer.toString(i)); + } + + ModifiableSolrParams params = new ModifiableSolrParams(); + params.set("taskUUID", "5"); + + var request = + new GenericSolrRequest( Review Comment: When I see `GSR`, generally that is an opportunity for us to use our SolrJ generated API instead. This gets us testing of the SolrJ response code for free! ########## solr/api/src/java/org/apache/solr/client/api/endpoint/ListActiveTasksApi.java: ########## @@ -0,0 +1,49 @@ +/* + * 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.api.endpoint; + +import static org.apache.solr.client.api.util.Constants.INDEX_PATH_PREFIX; + +import io.swagger.v3.oas.annotations.Operation; +import jakarta.ws.rs.GET; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.PathParam; +import org.apache.solr.client.api.model.ListActiveTaskResponse; +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 { + + // Handles: .../tasks/list (Lists all) + @GET + @StoreApiParameters + @Operation( + summary = "Lists all the currently running tasks", + tags = {"tasks"}) + ListActiveTaskResponse listAllActiveTasks() throws Exception; + + // Handles: .../tasks/list/slow-task-id (Lists specific) Review Comment: i don't think we need this comment, and I think this should be `GET /tasks/taskId` ########## solr/api/src/java/org/apache/solr/client/api/model/TaskStatusResponse.java: ########## @@ -0,0 +1,24 @@ +/* + * 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.api.model; + +import com.fasterxml.jackson.annotation.JsonProperty; + +public class TaskStatusResponse extends SolrJerseyResponse { + @JsonProperty public String taskStatus; Review Comment: maybe an oppotunity to use an enum for the various taskStatus???? -- 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]
