FrankChen021 commented on code in PR #20505:
URL: https://github.com/apache/druid/pull/20505#discussion_r4259553833
##########
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:
[P1] Align the local publish wait timeout
**Finding:** Streaming transactional actions now return a future that may
wait for predecessor completion, but SequenceMetadata still invokes
TaskActionClient.submit and LocalTaskActionClient.performAction still waits on
performAsync(...).get(5, TimeUnit.MINUTES). A predecessor can legitimately
remain pending until the supervisor completion timeout, which defaults to 30
minutes, so a valid task fails after five minutes instead of continuing. The
timeout does not cancel the future, leaving its listener active and allowing it
to invoke publish after the task has failed.
**Suggestion:** Give this wait an action-specific deadline tied to the
supervisor completion timeout and cancel it on local timeout, or keep the long
wait out of the fixed five-minute synchronous path.
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordResource.java:
##########
@@ -506,34 +518,57 @@ 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 {
+ final Map<String, String> payload = new HashMap<>();
Review Comment:
[P2] Restore generic action failure logging
**Finding:** The incremental fix restores the generic 500 error payload, but
this branch still does not log the throwable. Generic async action failures
therefore lose the stack trace and server-side diagnostics that the previous
synchronous handler emitted with log.warn, even though the client receives the
message.
**Suggestion:** Log the throwable in the generic onFailure branch before
completing the response.
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java:
##########
@@ -2945,27 +3021,62 @@ public boolean
isAnotherTaskGroupPublishingToPartitions(String taskId, Set<Objec
});
if (partitionIdToPublishingGroups.isEmpty()) {
- return false;
+ return Map.of();
}
// Check if any of the partitions of this taskId are being published by
other taskGroups
+ final Map<TaskGroup, Set<PartitionIdType>> blockingTaskGroups = new
HashMap<>();
for (Object partition : partitions) {
@SuppressWarnings("unchecked")
final PartitionIdType partitionId = (PartitionIdType) partition;
if (partitionIdToPublishingGroups.containsKey(partitionId)) {
- log.info(
- "Task[%s] needs to wait before publishing as other taskGroups[%s]
are currently publishing to partition[%s].",
- taskId,
- partitionIdToPublishingGroups.get(partitionId),
- partitionId
- );
- return true;
+ for (TaskGroup taskGroup :
partitionIdToPublishingGroups.get(partitionId)) {
Review Comment:
[P1] Later pending generations can deadlock
**Finding:** Once maxAllowedStops or taskCount allows more than one pending
group, pendingCompletionTaskGroups can contain older and newer generations for
the same partition. This loop adds every overlapping group except groups
containing the task ID; it never filters to predecessors. If an older task is
still pushing when a newer group reaches publish, the older publish can wait
for the newer group while the newer publish waits for the older one. Neither
can complete until the completion timeout removes both, stalling ingestion and
causing task restarts.
**Suggestion:** Associate the task with its pending TaskGroup or generation
and register listeners only for groups that precede it; add coverage with two
pending generations and a delayed first publish.
--
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]
