DanielLeens commented on PR #11458:
URL: https://github.com/apache/seatunnel/pull/11458#issuecomment-5477025558
Thanks @SEZ9 for continuing to track this, and @davidzollo for the rebuttal
— since this is Zeta's master-failover/slot-reuse path, I traced the full call
graph myself before taking a side rather than deferring to either comment. No
new commit landed since my last re-verification (`ebebc0b085bd`), so this is a
reply, not a new review.
**Issue 1 (F1, marked Blocking): I don't think this is reachable, and I've
traced the exact reason why.** The claim is that the `isSubPlan` branch of
`preApplyResources` never clears `masterFailoverRestore`, so it stays armed for
the JobMaster's whole lifetime and can fire on an ordinary later pipeline
restart. Structurally that line is correct — `masterFailoverRestore = false;`
only sits in the `!isSubPlan` success branch (`JobMaster.java:601`). But the
call graph guarantees that branch always runs, and succeeds, before the
`isSubPlan` branch can ever be reached for the same job:
```
CoordinatorService.pendingJobSchedule (only preApplyResources() caller with
subPlan=null)
-> jobMaster.preApplyResources() [isSubPlan =
false]
-> on enoughResource: physicalPlan.setPreApplyResourceFutures(futures)
masterFailoverRestore = false
(JobMaster.java:601)
-> only THEN does jobMaster.run() let pipelines progress CREATED ->
SCHEDULED
SubPlan.stateProcess(), case SCHEDULED
-> ResourceUtils.applyResourceForPipeline(jobMaster, this)
-> reads jobMaster.getPhysicalPlan().getPreApplyResourceFutures() (the
SAME map
populated by the whole-job call above) -> DEPLOYING -> RUNNING
SubPlan.stateProcess(), case FAILED/CANCELED (later, ordinary pipeline
restart)
-> checkNeedRestore(state) && prepareRestorePipeline()
-> jobMaster.preApplyResources(this) [isSubPlan =
true, SubPlan.java:741]
```
A pipeline can only reach the `FAILED`/`CANCELED` restart branch that calls
`preApplyResources(this)` after it has already been through
`DEPLOYING`/`RUNNING` at least once — and that first `DEPLOYING` transition
(`case SCHEDULED`) only happens by consuming futures that
`applyResourceForPipeline` reads out of
`physicalPlan.getPreApplyResourceFutures()`, which is only non-empty once the
whole-job `preApplyResources()` call has already succeeded and, in the same
success branch, cleared `masterFailoverRestore`. So by the time any
`isSubPlan=true` call is reachable, the flag is already `false`, and
`getReusableSlot()`'s `if (!masterFailoverRestore) { return null; }` gate has
already closed. I don't see a path where a fresh JobMaster's pipeline reaches a
restart-eligible state without first going through the whole-job call.
@davidzollo's read matches what I traced independently.
I'd still take the low-risk hardening for free, though: clearing the flag in
the `isSubPlan` success branch too (or once the job reaches RUNNING) costs
nothing and removes the dependency on this ordering guarantee holding forever
as the scheduler code evolves. I'd suggest keeping that as a Low-severity,
non-blocking follow-up rather than a merge blocker — it's defense-in-depth, not
a live bug on this head.
**Issues 2-8:** no new information in this round beyond what was already in
the previous pass — I'll leave those as the Medium/Low non-blocking follow-ups
they already were rather than re-litigating each one here, since nothing about
them changed with this reply.
With Issue 1 resolved as non-reachable, my merge recommendation from
`5063079162` stands: **ready to merge**, CI green, no blockers. As before, my
repository permission is comment-only for final sign-off, so a write-capable
maintainer should do the actual approve/merge.
--
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]
