[FLINK-39108][checkpoint] Respect disabled interval-during-backlog for the first checkpoint - #29110
Open
NestDream wants to merge 1 commit into
Open
[FLINK-39108][checkpoint] Respect disabled interval-during-backlog for the first checkpoint#29110NestDream wants to merge 1 commit into
NestDream wants to merge 1 commit into
Conversation
…r the first checkpoint When a source reported isProcessingBacklog=true while execution.checkpointing.interval-during-backlog was disabled, the periodic trigger armed by startCheckpointScheduler stayed scheduled: setIsProcessingBacklog only reschedules when the new effective interval is shorter and skips the disabled case entirely. On top of that, ScheduledTrigger#run stopped future scheduling when the effective interval was disabled but still triggered the current run. As a result at least one checkpoint fired during backlog processing even though checkpointing should have been suspended. Cancel the armed periodic trigger when the effective interval becomes disabled, and make ScheduledTrigger re-check the effective interval before triggering so a pending run is skipped once checkpointing is disabled. The trigger is re-armed by setIsProcessingBacklog when the backlog ends. This also re-enables CheckpointIntervalDuringBacklogITCase#testNoCheckpointDuringBacklog, which was disabled waiting for this fix (its annotation referenced FLINK-39018, a typo for FLINK-39108).
Contributor
Author
|
@rkhachatryan this re-enables testNoCheckpointDuringBacklog, the test you disabled in #27630. Could you take a look when you get a chance? |
Collaborator
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-39108.
execution.checkpointing.interval-during-backlogis honored for steady-state scheduling but not for the first checkpoint. Two things combine to let it through:ScheduledTrigger#runparks itself when it finds the effective interval disabled, but still triggers the current run. This is the path the disabled ITCase hits: the source reports backlog before its task is running, the scheduler is (re)started at task RUNNING with a zero initial delay, and that first run fires a checkpoint before parking.CheckpointCoordinator#setIsProcessingBacklogonly reschedules the periodic trigger when the new effective interval would fire earlier and skips the disabled case, so a trigger that is already armed when the backlog is reported stays armed and wakes up only to park.Net effect: at least one checkpoint is triggered during backlog even when checkpointing during backlog is disabled, which is what the ticket and the disabled
CheckpointIntervalDuringBacklogITCase#testNoCheckpointDuringBacklogdescribe.Brief change log
ScheduledTrigger#runno longer triggers a checkpoint when it finds the effective interval disabled; it only parkssetIsProcessingBacklogcancels an already armed periodic trigger when the effective interval becomes disabled instead of letting it wake up and park; when the backlog ends, the existing reschedule-if-earlier path re-arms it (the parked trigger's next fire time isLong.MAX_VALUE), so the first checkpoint after a backlog comes one full interval after the backlog endsCheckpointIntervalDuringBacklogITCase#testNoCheckpointDuringBacklog; the@Disabledannotation referenced FLINK-39018, a typo for FLINK-39108Verifying this change
This change added tests and can be verified as follows:
CheckpointIntervalDuringBacklogITCase#testNoCheckpointDuringBacklog(the test disabled in [FLINK-34099][tests] Harden and disable CheckpointIntervalDuringBacklogITCase #27630) reproduces the reported behavior against unmodified master: it fails withExpecting AtomicInteger(2) to have value: 0, i.e. two checkpoints ran while the first source was still in backlog. With the change the whole class passes, 4/4.CheckpointCoordinatorTriggeringTest#testFirstScheduledCheckpointNotTriggeredWhenBacklogCheckpointingDisabled: new unit test for a periodic trigger that is already armed when the backlog is reported. It checks that the trigger is cancelled right away, that no checkpoint is triggered during the backlog, and that exactly one checkpoint is triggered once the backlog ends. Fails on master at the first assertion (the trigger stays armed), passes with the change.CheckpointCoordinatorTriggeringTest#testSchedulerStartedDuringBacklogDoesNotTriggerWhenBacklogCheckpointingDisabled: new unit test for the ordering the ITCase hits (backlog reported before the scheduler is started). It checks that the first run of the armed trigger does not trigger a checkpoint and that checkpointing resumes once the backlog ends. Fails on master (one checkpoint is triggered), passes with the change.Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation