[FLINK-40234][runtime] Exclude synchronous downstream processing from source idleness accounting - #29152
Draft
slachiewicz wants to merge 1 commit into
Draft
Conversation
… source idleness accounting Everything a source emits is processed by the chained operators on the task thread before the reader is polled again. That time counted towards per-split idleness, so a slow chained operator or a window firing that outlasted the idle timeout marked splits with fetched-but-unpolled records idle; the combined watermark then advanced without them and their records were dropped as late. The source's DataOutput is now wrapped so the input activity clock is paused while downstream processing runs, mirroring the backpressure pause of FLIP-471. Per-split clocks are layered on the main clock so one pause covers all splits. The wrapper is only installed when a watermark strategy asked for the input activity clock (withIdleness); other jobs keep the unwrapped output. Generated-by: Claude Fable 5.1
Collaborator
Contributor
|
@slachiewicz I think we're a bit premature with this PR, don't forget to also take a look at https://flink.apache.org/how-to-contribute/contribute-code/#1-create-jira-ticket-and-reach-consensus. You are opening a lot of PRs and we're happy with the contributions, but this is a hard problem that first needs consensus on how it should be resolved. Like this PR says, there is no consensus/working solution yet. Then we shouldn't open a PR. A problem like this can only be picked up when consensus has been reached and an assignee has been set, like the guide says |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is the purpose of the change
Fixes FLINK-40234. Per-split idleness detection in
SourceOperatormeasures time on aPausableRelativeClockthat FLIP-471 pauses during backpressure and watermark-alignment pauses. Time the task thread spends inside the chained operators was still counted: a slow chained operator, or a window firing triggered by a watermark, that outlasts the idle timeout gets a split with fetched-but-unpolled records marked idle. The combined watermark then advances without that split and its records are dropped as late.This PR pauses the input activity clock while downstream processing runs, the same treatment FLIP-471 gives to backpressure.
Brief change log
PausableRelativeClockaccepts anyRelativeClockas base, so per-split clocks are now layered on the operator's main clock and one pause covers every split. The per-split backpressure listener registration goes away because the main clock, registered inSourceOperator#open, already provides it.ActivityClockPausingDataOutputwraps the source'sDataOutputand pauses the main clock around every downstream emit call (records, watermarks, statuses, latency markers, record attributes).WatermarkStrategy#withIdlenessdoes. Jobs without idleness keep the unwrapped output and an unchanged hot path.Behaviour changes worth knowing
fof the task thread, detecting a genuinely empty split next to a streaming one takestimeout / (1 - f)wall time instead oftimeout. This trades slower idle detection for not dropping records, consistent with how backpressured time is already treated.WAITING_FOR_ALIGNMENT), which previously only froze the main clock. No split is polled in that mode.pause/unPausepair, measured at about 36 ns per element on an Apple M1 under JDK 17 (uncontended monitor plus twoSystem.nanoTimereads). No cost for jobs without idleness.Direction requested
This is the operator-level layer of the fix, and I would like the assignee's and reporter's view on whether it is the layer you want before polishing further:
timeout / (1 - f)) and the per-record cost above for idleness users.SourceOperatorSplitWatermarkAlignmentTestencode (they advance time with records still queued in the mock reader and expect idleness), so those tests would need to be rewritten.SourceReaderBaseknows when it holds fetched-but-unemitted records for a split and could keep that split's activity clock paused. That is exact per split and has no per-record cost, but needs a way to enumerate the split ids of a queuedRecordsWithSplitIdsand an internal hook from the reader to the split's clock, so it is a connector-base API change.I lean towards 1 now and 3 as the follow-up. Happy to reshape this PR either way.
Verifying this change
PausableRelativeClockTest#layeredClockFollowsBaseClockPausescovers clock layering.ActivityClockPausingDataOutputTestcovers the decorator, including resuming the clock when the downstream call throws.SourceOperatorSplitWatermarkAlignmentTest#testSlowDownstreamRecordProcessingDoesNotMarkUnpolledSplitIdleand#testSlowDownstreamWatermarkProcessingDoesNotMarkUnpolledSplitIdlereproduce both scenarios from the ticket at the operator level. Both fail on master withExpecting value to be false but was trueon the split'sisIdle()after the busy stretch.Verified:
mvn -pl flink-runtime test -Dtest=SourceOperatorSplitWatermarkAlignmentTest,ActivityClockPausingDataOutputTest,PausableRelativeClockTest→ 21 tests, 0 failures;spotless:checkandcheckstyle:checkclean (JDK 17, rebased on masterc31f46757a4).Does this pull request potentially affect one of the following parts
@InternalDocumentation
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Fable 5.1