Skip to content

fix: Do not block in ChainedProcessorManager.next(). - #20321

Open
gianm wants to merge 1 commit into
apache:masterfrom
gianm:msq-chainedproc-next-block
Open

gianm wants to merge 1 commit into
apache:masterfrom
gianm:msq-chainedproc-next-block

Conversation

@gianm

@gianm gianm commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Previously, a blocking Future wait ran inside next(), which could cause a processing thread to block while waiting for the next processor to be ready. This patch fixes it by moving resolution to a callback.

Previously, a blocking Future wait ran inside next(), which could
cause a processing thread to block while waiting for the next processor
to be ready. This patch fixes it by moving resolution to a callback.
@github-actions github-actions Bot added Area - Batch Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Sep 10, 2026
@FrankChen021
FrankChen021 self-requested a review September 11, 2026 02:21

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The change removes the blocking wait from the first phase and synchronizes the phase transition correctly. One cleanup race remains in the asynchronous rest phase: after the rest manager has been created, a pending rest.next() can resolve after close and return a processor that is no longer owned by the executor. That processor needs the same post-close cleanup handling as processors returned by the first phase.

Reviewed 2 of 2 changed files. This was a static review; no local build or test commands were run.

Severity Findings
P0 0
P1 0
P2 1
P3 0
Total 1

This is an automated review by Codex GPT-5.6 Luna(Max)

After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.

private ListenableFuture<Optional<ProcessorAndCallback<Object>>> nextFromRest()
{
//noinspection unchecked, rawtypes
return FutureUtils.transformAsync(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Clean up rest processors that arrive after close

Once restFuture has already been resolved, close() cannot cancel it; a pending rest.next() can therefore still resolve with a ProcessorAndCallback after the manager is closed. nextFromRest() forwards that value without checking closed or cleaning up the returned FrameProcessor, unlike the first-phase callback above. During a failure or cancellation, RunAllFullyWidget can discard this late processor after its manager has been closed, leaving its channels or other resources uncleaned. Route rest.next() through the same post-close cleanup path (or otherwise cancel/track the in-flight future).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area - Batch Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants