davidzollo commented on PR #12029: URL: https://github.com/apache/seatunnel/pull/12029#issuecomment-5552090570
@DanielLeens Thanks for the review — sorry for the delay in closing this out. Both actionable points from your review are addressed in `16c5820da`, and the branch is now synced with current `dev`. ### Issue 1 (Medium) — redeploy-switch misattribution: fixed, and your reading of the call chain is correct I re-traced it against current `dev` before touching the text: - `PhysicalVertex#initStateFuture()` (`PhysicalVertex.java:182-205`) restores the persisted state and runs `checkTaskGroupIsExecuting` for RUNNING/DEPLOYING, only ever downgrading to FAILING. - The restored state is then replayed per vertex by `PhysicalVertex#restoreExecutionState()` (`PhysicalVertex.java:207-210`), which calls `PhysicalVertex#stateProcess()`. - `PhysicalVertex#stateProcess()` (`PhysicalVertex.java:583`) is the switch on `ExecutionState`: `case INITIALIZING/CREATED/RUNNING` falls through to `break` (no redeploy), and `case DEPLOYING` (`:591`) unconditionally calls `deploy(jobMaster.getOwnedSlotProfiles(taskGroupLocation))` — the pre-fix crash site. - `SubPlan#stateProcess()` (`SubPlan.java:659`) switches on `PipelineStatus`, not `ExecutionState`, so it was the wrong owner for that branch. The Javadoc now names `PhysicalVertex#stateProcess` for both the `case RUNNING` no-op and the `case DEPLOYING` redeploy, and I made the two remaining bare `stateProcess` references explicit for the same reason. One nuance worth keeping on the record: the `SubPlan#stateProcess` reference in the *second* paragraph was not wrong, it was just imprecise about which layer does what, so I kept it and sharpened it rather than deleting it. `SubPlan#stateProcess`'s `case DEPLOYING` (`SubPlan.java:702-717`) really does own the single-threaded fan-out — it `forEach`es the vertices and calls `makeTaskGroupDeploy()`, which is `updateTaskState(ExecutionState.DEPLOYING)` (`PhysicalVertex.java:303-305`), which writes the IMap and then synchronously calls `PhysicalVertex#stateProcess()` and its deploy RPC before the loop moves to the next vertex. That is what makes the watcher's assumption hold, so the text now says exactly that: SubPlan drives the fan-out, PhysicalVertex issues the deploy. It also means a vertex becomes visibly DEPLOYING *just before* its RPC goes out (the IMap write precedes `stateProcess`), not once the RPC is already in flight — corrected that too. ### Issue 2 (Low) — shared deadline: fixed `deployingObserved.get(...)` now waits 60s against the watcher's own 30s polling deadline, with a comment explaining why the two must not match. You were right about the failure mode: on a genuine miss the outer `get` raced `killActiveMasterWhileAnyVertexDeploying`'s normal `false` return, so a missed trigger window would most often surface as a bare `TimeoutException` and throw away the diagnostic message that exists precisely to tell a maintainer "the window was missed, this run did not exercise the fix" rather than "the fix is broken". ### Issue 3 (Low) — duplicated helpers: deliberately deferred Agreed on the substance, but extracting the four helpers into shared test infrastructure now would mean touching `SplitClusterPendingJobLifecycleFailoverIT` as well, which is a file #12027 is actively changing and which #12034 has already landed into. Doing it here would create exactly the kind of cross-PR merge conflict this batch has already hit once. Better as a follow-up once the batch has landed — no objection to it, just not in this PR. ### Status `Build` is re-running against the new head. The previous head (`28028bdc9`) was fully green including both `engine-v2-it` jobs, and this change is comment-only plus one timeout constant, so I do not expect behavioural movement — but I will confirm on the new run rather than assume. -- 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]
