From 0a279e634a0cdd0fdc29ce3774fedaf7678ed025 Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Tue, 8 Oct 2024 03:19:34 -0700 Subject: [PATCH] DartSqlResource: Return HTTP 202 on cancellation even if no such query. (#17278) Return HTTP 202 (Accepted) on cancellation, even if the requested query ID was not found. The main reason for this is that when the Router broadcasts DELETE requests to all Brokers, it returns the response from one of them randomly. If we return 404 when a query ID isn't found, then the Router randomly returns 404s even when the query really was found and canceled. This is also arguably still correct behavior. The cancellation request *was* accepted, it just won't do anything because the query was not in fact running. --- .../msq/dart/controller/http/DartSqlResource.java | 11 +++++++---- .../msq/dart/controller/http/DartSqlResourceTest.java | 2 +- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/dart/controller/http/DartSqlResource.java b/extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/dart/controller/http/DartSqlResource.java index ef6725d455c4..65d770a29c55 100644 --- a/extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/dart/controller/http/DartSqlResource.java +++ b/extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/dart/controller/http/DartSqlResource.java @@ -239,7 +239,10 @@ public Response cancelQuery( List cancelables = sqlLifecycleManager.getAll(sqlQueryId); if (cancelables.isEmpty()) { - return Response.status(Response.Status.NOT_FOUND).build(); + // Return ACCEPTED even if the query wasn't found. When the Router broadcasts cancellation requests to all + // Brokers, this ensures the user sees a successful request. + AuthorizationUtils.setRequestAuthorizationAttributeIfNeeded(req); + return Response.status(Response.Status.ACCEPTED).build(); } final Access access = authorizeCancellation(req, cancelables); @@ -249,14 +252,12 @@ public Response cancelQuery( // Don't call cancel() on the cancelables. That just cancels native queries, which is useless here. Instead, // get the controller and stop it. - boolean found = false; for (SqlLifecycleManager.Cancelable cancelable : cancelables) { final HttpStatement stmt = (HttpStatement) cancelable; final Object dartQueryId = stmt.context().get(DartSqlEngine.CTX_DART_QUERY_ID); if (dartQueryId instanceof String) { final ControllerHolder holder = controllerRegistry.get((String) dartQueryId); if (holder != null) { - found = true; holder.cancel(); } } else { @@ -269,7 +270,9 @@ public Response cancelQuery( } } - return Response.status(found ? Response.Status.ACCEPTED : Response.Status.NOT_FOUND).build(); + // Return ACCEPTED even if the query wasn't found. When the Router broadcasts cancellation requests to all + // Brokers, this ensures the user sees a successful request. + return Response.status(Response.Status.ACCEPTED).build(); } else { return Response.status(Response.Status.FORBIDDEN).build(); } diff --git a/extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/dart/controller/http/DartSqlResourceTest.java b/extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/dart/controller/http/DartSqlResourceTest.java index 8a8ce5fd3e86..51e17235203d 100644 --- a/extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/dart/controller/http/DartSqlResourceTest.java +++ b/extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/dart/controller/http/DartSqlResourceTest.java @@ -727,7 +727,7 @@ public void test_cancelQuery_regularUser_unknownQuery() .thenReturn(makeAuthenticationResult(REGULAR_USER_NAME)); final Response cancellationResponse = sqlResource.cancelQuery("nonexistent", httpServletRequest); - Assertions.assertEquals(Response.Status.NOT_FOUND.getStatusCode(), cancellationResponse.getStatus()); + Assertions.assertEquals(Response.Status.ACCEPTED.getStatusCode(), cancellationResponse.getStatus()); } /**