epugh commented on code in PR #4828:
URL: https://github.com/apache/solr/pull/4828#discussion_r3889375887


##########
solr/core/src/resources/ImplicitPlugins.json:
##########
@@ -144,10 +144,10 @@
       }
     },
     "/tasks/cancel": {
-      "class": "solr.QueryCancellationHandler",
+      "class": "solr.TaskCancellationHandler",
       "useParams":"_TASK_CANCELLATION",

Review Comment:
   Okay, caan you look the components that are defined in this?   I poked 
aroudn a bit and I think that the `"components"` defined in this file aren't 
actually needed anymore or used....   Both for the cancel handler and the list 
tasks handler below..  Can you investigate and see if this is old dead code, 
maybe from our previous work to migrate tasks?   I don't think it breaks 
anything, but since we are in here, we should clean this up.



##########
solr/core/src/java/org/apache/solr/handler/admin/api/CancelTask.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * 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 jakarta.inject.Inject;
+import org.apache.solr.api.JerseyResource;
+import org.apache.solr.client.api.endpoint.TasksApi;
+import org.apache.solr.client.api.model.CancelTaskResponse;
+import org.apache.solr.handler.component.ActiveTaskQuerySupport;
+import org.apache.solr.jersey.PermissionName;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.CancellableCollector;
+
+public class CancelTask extends JerseyResource implements TasksApi.Cancel {
+
+  private final SolrQueryRequest solrQueryRequest;
+
+  @Inject
+  public CancelTask(SolrQueryRequest solrQueryRequest) {
+    this.solrQueryRequest = solrQueryRequest;
+  }
+
+  @Override
+  @PermissionName(READ_PERM)
+  public CancelTaskResponse cancelRunningTask(String taskID) throws Exception {

Review Comment:
   Can you dig a bit more..  I did a cURL and I get back HTTP 200, though with 
a 404 in the actual TEXT of the response.   I am going to push up a unit test 
for this class that should illustrate the 404.



##########
solr/core/src/java/org/apache/solr/handler/admin/api/CancelTask.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * 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 jakarta.inject.Inject;
+import org.apache.solr.api.JerseyResource;
+import org.apache.solr.client.api.endpoint.TasksApi;
+import org.apache.solr.client.api.model.CancelTaskResponse;
+import org.apache.solr.handler.component.ActiveTaskQuerySupport;
+import org.apache.solr.jersey.PermissionName;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.CancellableCollector;
+
+public class CancelTask extends JerseyResource implements TasksApi.Cancel {
+
+  private final SolrQueryRequest solrQueryRequest;
+
+  @Inject
+  public CancelTask(SolrQueryRequest solrQueryRequest) {
+    this.solrQueryRequest = solrQueryRequest;
+  }
+
+  @Override
+  @PermissionName(READ_PERM)
+  public CancelTaskResponse cancelRunningTask(String taskID) throws Exception {

Review Comment:
   The unit test I added has a great big javadoc, that we should cut down to a 
minimum or remove once you have the fix in!  testCancelNonExistentTaskReturns404



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

Reply via email to