FrankChen021 commented on code in PR #20505:
URL: https://github.com/apache/druid/pull/20505#discussion_r4237635164
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordResource.java:
##########
@@ -506,34 +517,55 @@ public Response getWorkerConfigHistory(
@Path("/action")
@Produces(MediaType.APPLICATION_JSON)
@ResourceFilters(StateResourceFilter.class)
- public Response doAction(final TaskActionHolder holder)
+ public void performTaskAction(
+ final TaskActionHolder holder,
+ @Context HttpServletRequest request
+ )
{
- return asLeaderWith(
- taskMaster.getTaskActionClient(holder.getTask()),
- new Function<>()
+ final AsyncContext asyncContext = request.startAsync();
+ final Optional<TaskActionClient> taskActionClient =
taskMaster.getTaskActionClient(holder.getTask());
+ if (!taskActionClient.isPresent()) {
+ // Encourage client to try again soon, when we'll likely have a redirect
set up
+ completeAsyncRequest(asyncContext,
Status.SERVICE_UNAVAILABLE.getStatusCode(), null);
+ return;
+ }
+
+ final ListenableFuture<?> future =
taskActionClient.get().submitAsync(holder.getAction());
+ asyncContext.addListener(
+ ServletResourceUtils.createAsyncTimeoutListener(event -> {
+ future.cancel(true);
+ completeAsyncRequest(event.getAsyncContext(),
HttpResponseStatus.GATEWAY_TIMEOUT.code(), null);
Review Comment:
[P1] Cancellation can overwrite the timeout response
**Finding:** The timeout handler cancels the future before completing the
504 response. Because the callback uses directExecutor, cancel(true) can
synchronously invoke onFailure with CancellationException; that branch treats
cancellation as a generic 500 and completes the context before the timeout
handler writes 504. A near-simultaneous completion can also cause duplicate
response completion.
**Suggestion:** Use an atomic response-ownership guard, treat cancellation
as the timeout path, and prevent late callbacks from writing after the 504
response.
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordResource.java:
##########
@@ -506,34 +517,55 @@ public Response getWorkerConfigHistory(
@Path("/action")
@Produces(MediaType.APPLICATION_JSON)
@ResourceFilters(StateResourceFilter.class)
- public Response doAction(final TaskActionHolder holder)
+ public void performTaskAction(
+ final TaskActionHolder holder,
+ @Context HttpServletRequest request
+ )
{
- return asLeaderWith(
- taskMaster.getTaskActionClient(holder.getTask()),
- new Function<>()
+ final AsyncContext asyncContext = request.startAsync();
+ final Optional<TaskActionClient> taskActionClient =
taskMaster.getTaskActionClient(holder.getTask());
+ if (!taskActionClient.isPresent()) {
+ // Encourage client to try again soon, when we'll likely have a redirect
set up
+ completeAsyncRequest(asyncContext,
Status.SERVICE_UNAVAILABLE.getStatusCode(), null);
+ return;
+ }
+
+ final ListenableFuture<?> future =
taskActionClient.get().submitAsync(holder.getAction());
Review Comment:
[P1] Initial publish runs before timeout setup
**Finding:** submitAsync is invoked before the timeout listener and
15-minute timeout are registered. Local streaming publish actions execute their
first metadata publish synchronously inside submitAsync, so a slow first
attempt is governed by the container's default timeout and cannot be canceled
through this listener.
**Suggestion:** Install the timeout and one-shot completion state before
starting the action, or move the initial publish attempt off the request thread.
--
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]
