[FLINK-40379][runtime] Wait for pre-restart parallelism to become available again after a rescale-triggered restart - #28999
[FLINK-40379][runtime] Wait for pre-restart parallelism to become available again after a rescale-triggered restart#28999akalash wants to merge 3 commits into
Conversation
…ilable again after a rescale-triggered restart Adds jobmanager.adaptive-scheduler.rescale.resource-stabilization-timeout, bounding how long the JobManager waits, after a rescale-triggered restart, for the pre-restart target parallelism to become available again from genuinely free slots before proceeding with whatever is sufficient. Previously, a restart triggered by a resource change would fall back to "sufficient resources" as soon as a single slot was free, even if the slots backing the just-cancelled execution had not been released yet, causing avoidable churn back to a lower parallelism.
…scheduler.rescale.resource-stabilization-timeout
| new WaitingForResources.Factory( | ||
| this, | ||
| LOG, | ||
| settings.getSubmissionResourceWaitTimeout(), |
There was a problem hiding this comment.
Side note: It seems we use SubmissionResourceWaitTimeout for all cases - not only for submission. Perhaps we should follow-up on that with something like:
previousExecutionGraph == null
? settings.getSubmissionResourceWaitTimeout()
: Duration.ofMillis(-1L)
XComp
left a comment
There was a problem hiding this comment.
Thanks for the fix, @akalash . I did a pass over the change and have a few comments. PTAL
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: yes (changes the adaptive scheduler's restart/rescaling behavior in the JobManager)
The PR template question is not properly answered
There was a problem hiding this comment.
We should not keep the docs change separate because it's correlated with the JobManagerOptions change of the previous commit. But that's a quick fix by just running squash & rebase on the branch when merging
|
|
||
| ctx.setAvailableVertexParallelism(availableParallelism); | ||
| ctx.setAchievableVertexParallelism(parallelismBasedOnFreeSlots); | ||
| ctx.setHasDesiredResources(hasDesiredResources); |
There was a problem hiding this comment.
Here, I'm wondering whether we're misaligned: We set the parallelism based on free slots to 1 even though the job requires parallelism of 2 to transition to ExecutionGraph creation.
The ExecutionGraph creation also relies on free slots (see [AdaptiveScheduler:1259(https://github.com/apache/flink/blob/e93c3ef695a0f27950623186b369507e05aeeb54/flink-runtime/src/main/java/org/apache/flink/runtime/scheduler/adaptive/AdaptiveScheduler.java#L1259). That backs the observation that we should also rely on free slots for the desired resources to avoid creating the EG with less slots again.
There was a problem hiding this comment.
Yes, it's known as another bug. The goal is to fix it in a follow-up since it's pretty much independent of the current changes, so it's better to keep it separate to avoid confusion
There was a problem hiding this comment.
Can you reference the FLINK Jira issue?
| // only 1 of the 2 targeted slots is free: must not have shortcut to | ||
| // CreatingExecutionGraph yet, even though 1 slot is already "sufficient" to run the job | ||
| // at a lower parallelism. | ||
| Thread.sleep(300); |
There was a problem hiding this comment.
can't we wait here for the state to transition to WaitingForResources instead of setting a hard-coded sleep?
There was a problem hiding this comment.
Yep, we don't need to wait here
XComp
left a comment
There was a problem hiding this comment.
Thanks for addressing my comments. I did another pass. PTAL
| @Nullable VertexParallelism expectedTargetVertexParallelism) { | ||
| waitingForResourcesStateValidator.expectInput( | ||
| arguments -> { | ||
| assertNonNull().accept(arguments); |
There was a problem hiding this comment.
| assertNonNull().accept(arguments); | |
| assertThat(arguments.getExecutionGraph()).isNotNull(); |
The initial implementation validated that the executionGraph isn't null
| return context.runIfState(this, callback, delay); | ||
| } | ||
|
|
||
| private boolean isFreeSlotVertexParallelismAtLeast(VertexParallelism target) { |
There was a problem hiding this comment.
| private boolean isFreeSlotVertexParallelismAtLeast(VertexParallelism target) { | |
| private boolean hasFreeSlotsFor(VertexParallelism target) { |
nit: what about that method name? Otherwise, maybe isAtLeastFreeSlotVertexParallelism
| // the target is never reached, but the stabilization timeout must still force the | ||
| // transition once it elapses, rather than waiting forever. | ||
| CommonTestUtils.waitUntilCondition( | ||
| () -> scheduler.getState() instanceof CreatingExecutionGraph, |
There was a problem hiding this comment.
| () -> scheduler.getState() instanceof CreatingExecutionGraph, | |
| () -> !(scheduler.getState() instanceof WaitingForResources). |
AdaptiveScheduler is transitioning to Executing eventually which might make this condition time out occasionally if we miss the time window of the CreatingExecutionGraph state
|
|
||
| ctx.setAvailableVertexParallelism(availableParallelism); | ||
| ctx.setAchievableVertexParallelism(parallelismBasedOnFreeSlots); | ||
| ctx.setHasDesiredResources(hasDesiredResources); |
There was a problem hiding this comment.
Can you reference the FLINK Jira issue?
| log, | ||
| backoffTime, | ||
| restartWithParallelism, | ||
| targetVertexParallelism, |
There was a problem hiding this comment.
| targetVertexParallelism, | |
| restartWithParallelism, |
We have to be consistent here - I'm ok with either leaving restartWithParallelism in the Restarting class because it's actually covering the restarting context. Or we rename all the occurrences to targetVertexParallelism.
What is the purpose of the change
When the adaptive scheduler restarts a job to change its parallelism (a rescale), it previously fell back to "sufficient resources" as soon as a single slot became free — even if the slots backing the just-cancelled execution had not been released yet. This caused avoidable churn: the job would restart at a lower parallelism than what was actually about to become available, only to be rescaled again shortly after - in the worst case, when the slot is canceled by timeout(or failed during the cancellation), it can lead to an infinite restarting loop: (for 1 -> 2 scaling) JM sees 2 slots -> cancel the current execution -> one slot fail -> the job restart with 1 slot only -> JM sees 2 slots again -> ...
This pull request makes the JobManager wait, after a rescale-triggered restart, for the pre-restart target parallelism to become available again from genuinely free slots, bounded by a new configurable timeout, before proceeding with whatever is sufficient.
Brief change log
jobmanager.adaptive-scheduler.rescale.resource-stabilization-timeout, bounding how long the JobManager waits after a rescale-triggered restart for the pre-restart target parallelism to become available again.AdaptiveScheduler#goToWaitingForResourcesnow accepts the targetVertexParallelismof a restart and configures theWaitingForResourcesstate's stabilization phase with the new rescale timeout instead of the submission one when restarting.WaitingForResourcesgateshasDesiredResources()on the restart target parallelism (computed from genuinely free slots, excluding slots still reserved by the execution being cancelled) instead of the plain "sufficient resources" check, only for restarts with a known target.Restartingnow determines and passes through the target parallelism to restart with, viagetFreeSlotVertexParallelism()/getUpperBoundParallelism(...).Verifying this change
This change added tests and can be verified as follows:
AdaptiveSchedulerRescaleRestartTimingTest#testWaitingForResourcesDoesNotTransitionUntilFreeSlotsReachRescaleTargetverifies the scheduler keeps waiting until the restart target parallelism is reachable from free slots.AdaptiveSchedulerRescaleRestartTimingTest#testWaitingForResourcesFallsBackAfterRescaleResourceStabilizationTimeoutElapsesverifies the scheduler proceeds with whatever is sufficient once the new stabilization timeout elapses, rather than waiting forever.AdaptiveSchedulerFreeSlotVertexParallelismTest#testFreeSlotVertexParallelismExcludesReservedSlotsverifies free-slot-based parallelism calculation excludes slots still reserved by the execution being cancelled.AdaptiveSchedulerTest#testGoToWaitingForResourcesForRestartConfiguresStateTransitionManagerFactoryverifies the restart path configures the state transition manager with the rescale resource-stabilization timeout and skips the cooldown phase.WaitingForResourcesTest(e.g.testDesiredResourcesRequireReachingRestartTargetRegardlessOfBaseCheck,testDesiredResourcesCapRestartTargetToLatestResourceRequirements,testDesiredResourcesAreMetOnceFreeSlotParallelismReachesRestartTarget,testResourceTimeoutOverridesRestartTargetGuard) cover the new restart-target gating logic in isolation.CreatedTest,CreatingExecutionGraphTest, andRestartingTestwere extended to cover passing the restart target parallelism through the relevant state transitions.Does this pull request potentially affect one of the following parts:
@Public(Evolving): yes (a newConfigOptionwas added toJobManagerOptions, which is@PublicEvolving; purely additive, no existing options changed behavior)Documentation
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Sonnet 5)