FrankChen021 commented on code in PR #20505:
URL: https://github.com/apache/druid/pull/20505#discussion_r4235825815
##########
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 doAction(
+ 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 is outside the async timeout
**Finding:** This invokes submitAsync before the async listener and
15-minute timeout are installed. For the streaming publish actions handled by
this PR, submitAsync calls performAsync synchronously and
publishSegmentsWhenReady performs the first metadata/lock publish attempt
inline before it returns a future. If that database or lock operation stalls,
the servlet thread remains blocked and the request has no timeout or
cancellation path, defeating the new async endpoint and allowing stalled
publishes to consume Overlord request threads indefinitely.
**Suggestion:** Dispatch the initial publish through an executor or
otherwise obtain the future without running the first attempt on the servlet
thread, and install the timeout/cancellation handling so it covers that first
attempt as well.
##########
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 doAction(
+ 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);
Review Comment:
[P1] Timeout cancellation races the response callback
**Finding:** Canceling this future can synchronously invoke the onFailure
callback because the callback is registered with directExecutor below; that
callback treats cancellation as an internal-server-error response and completes
the AsyncContext before this handler tries to write the intended 504. A
concurrent success or failure can race the same timeout path as well, so one
request can produce a 500/200 instead of the promised gateway timeout and then
attempt a second completion on a closed response.
**Suggestion:** Serialize response completion with an atomic once guard, and
make the timeout path claim the response or ignore the cancellation callback
before canceling the underlying future.
--
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]
