Skip to content

Commit

Permalink
DartSqlResource: Return HTTP 202 on cancellation even if no such quer…
Browse files Browse the repository at this point in the history
…y. (apache#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.
  • Loading branch information
gianm authored Oct 8, 2024
1 parent 01baf99 commit 0a279e6
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,10 @@ public Response cancelQuery(

List<SqlLifecycleManager.Cancelable> 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);
Expand All @@ -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 {
Expand All @@ -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();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

/**
Expand Down

0 comments on commit 0a279e6

Please sign in to comment.