FrankChen021 commented on code in PR #20505:
URL: https://github.com/apache/druid/pull/20505#discussion_r4259557440
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/actions/TaskActionToolbox.java:
##########
@@ -140,25 +154,46 @@ public boolean canBatchSegmentAllocation()
}
/**
- * Checks if the given publish action should be failed without allowing any
- * more retries. A failed publish action should be retried only if there is
- * another task waiting to publish offsets for an overlapping set of
partitions.
+ * Performs the segment publish action when the given task is unblocked for
publish.
+ * A streaming task must wait for previously created tasks that are
+ * yet to publish offsets for an overlapping set of partitions.
*/
- public boolean shouldFailSegmentPublishImmediately(
- SegmentPublishResult result,
+ public ListenableFuture<SegmentPublishResult> publishSegmentsWhenReady(
Task task,
String supervisorId,
- DataSourceMetadata startMetadata
+ DataSourceMetadata startMetadata,
+ BiFunction<Task, TaskActionToolbox, SegmentPublishResult> publishAction
)
{
- if (result.isSuccess() || !result.isRetryable() || startMetadata == null) {
- return false;
+ // Try publishing later if the failure was due to offset mismatch
+ final ListenableFuture<Boolean> taskReadyToPublishFuture;
+ if (startMetadata == null || supervisorId == null) {
+ taskReadyToPublishFuture = Futures.immediateFuture(true);
+ } else {
+ taskReadyToPublishFuture =
supervisorManager.isTaskReadyToPublishSegments(
+ supervisorId,
+ task.getId(),
+ startMetadata
+ );
}
- return !getSupervisorManager().isAnotherTaskGroupPublishingToPartitions(
- supervisorId,
- task.getId(),
- startMetadata
+ return Futures.transform(
Review Comment:
The five-minute issue remains at the current head. SequenceMetadata still
calls TaskActionClient.submit, and LocalTaskActionClient.performAction still
waits on performAsync(...).get(5, TimeUnit.MINUTES), while the supervisor
completion timeout defaults to 30 minutes. A valid predecessor wait can
therefore fail the local streaming task after five minutes, and the timeout
path does not cancel the pending future, so it can retain the listener and
later attempt publication after the task has failed. Please align the action
timeout with the supervisor completion window and cancel the future on timeout,
or keep this long-lived wait out of the fixed synchronous path.
<!-- mergelens:review -->
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordResource.java:
##########
@@ -506,34 +518,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 AsyncRequest asyncRequest = new AsyncRequest(request);
+ 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
+ asyncRequest.complete(Status.SERVICE_UNAVAILABLE.getStatusCode(), null);
+ return;
+ }
+
+ final ListenableFuture<?> future =
taskActionClient.get().submitAsync(holder.getAction());
+ asyncRequest.context.addListener(
+ ServletResourceUtils.createAsyncTimeoutListener(_ -> {
+ asyncRequest.complete(HttpResponseStatus.GATEWAY_TIMEOUT.code(),
null);
+ future.cancel(true);
+ })
+ );
+
+ // Use a default timeout of 15 minutes
+ asyncRequest.context.setTimeout(15 * 60_000);
+
+ Futures.addCallback(
+ future,
+ new FutureCallback<Object>()
{
@Override
- public Response apply(TaskActionClient taskActionClient)
+ public void onSuccess(Object result)
{
- final Map<String, Object> retMap;
+ // Use null-safe map since some task actions may return null result
+ final Map<String, Object> payload = new HashMap<>();
+ payload.put("result", result);
- // It would be great to verify that this worker is actually
supposed to be running the task before
- // actually doing the action. Some ideas for how that could be
done would be using some sort of attempt_id
- // or token that gets passed around.
+ asyncRequest.complete(Status.OK.getStatusCode(), payload);
+ }
- try {
- final Object ret = taskActionClient.submit(holder.getAction());
- retMap = new HashMap<>();
- retMap.put("result", ret);
- }
- catch (Exception e) {
- log.warn(e, "Failed to perform task action");
- return Response.serverError().entity(ImmutableMap.of("error",
e.getMessage())).build();
+ @Override
+ public void onFailure(Throwable t)
+ {
+ if (t instanceof DruidException druidException) {
+ asyncRequest.complete(druidException.getStatusCode(),
druidException.toErrorResponse());
+ } else {
+
asyncRequest.complete(Status.INTERNAL_SERVER_ERROR.getStatusCode(), null);
Review Comment:
The response-payload part is fixed, but the generic failure branch still
omits the previous log.warn(Throwable, ...) call. The client now receives the
error message, yet the Overlord loses the stack trace and server-side
diagnostics for generic action failures. Please restore throwable logging
before completing the 500 response.
<!-- mergelens:review -->
--
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]
