DanielLeens commented on PR #12042: URL: https://github.com/apache/seatunnel/pull/12042#issuecomment-6472244326
Pushed `7cfd9a7c23` on top of the `dev` merge (`629016a3`). It only touches test fixtures, comments and docs; no production behavior changed relative to `2e03ae07`. **CI regression (nzw921rx, Issue 3).** Both `CheckpointCoordinatorTest` fixtures now use `new TaskLocation(new TaskGroupLocation(1L, 1, 1), 0, 0)` instead of a group-less `new TaskLocation()`; assertions are unchanged. The four `unit-test` jobs on `67e59e1e` failed on exactly these two `Null key is not allowed!` errors. The ubuntu/JDK17 job on that run also had `JobStateEventTest.testNotEndJobStateEvent` time out (`expected: <3> but was: <4>`); it did not reproduce in the other three matrix jobs or on `2e03ae07`, and this PR does not touch job events, so I did not change it. **Issue 1 (CLOSED not reconstructed in `pipelineTaskStatus`).** I checked the premise on this head: no production code reports `SeaTunnelTaskState.CLOSED` to the coordinator. `AbstractTask.reportTaskStatus` is only called with `WAITING_RESTORE` and `READY_START` (`SeaTunnelTask:265,273`, `SourceSplitEnumeratorTask:321,326`, `SinkAggregatedCommitterTask:141,146`), so `pipelineTaskStatus` never holds `CLOSED` before a failover either. `getActionStates()` / `getTaskStatistics()` therefore produce the same checkpoint shape on the old and the new master, and the new test's `0 acknowledged subtasks` assertion for the idle reader matches a pre-failover checkpoint. Reconstructing `CLOSED` on restore would introduce a shape that the normal path never produces, so I left it out. If you have a call site that reports `CLOSED` that I missed, please point me to it. **Issue 2 (second IMap read).** Kept the read in `restoreCoordinator`, since the coordinator has no `PhysicalVertex` access and threading the accepted group set through `CheckpointManager.reportedPipelineRunning` would widen a public signature. The `PhysicalVertex` Javadoc now says the single read is within `SubPlan`'s decision, and `restoreCoordinator` documents the re-read and why it is acceptable. **Issue 4 / 5 / F6 (docs).** The section is translated in the Chinese tree and moved into the recovery chapter as `4.3` in both trees, before "Related Resources" / "References". **Issue 6.** Added a comment at the `notifyCompleted` call in `restoreCoordinator` noting that finished groups receive `CheckpointFinishedOperation` through `TaskExecutionService#getExecutionContext`'s `finishedExecutionContexts` fallback, and that a lost worker fails the restore into the existing restart path. **Hunks for SEZ9 (all on `7cfd9a7c23`):** - F1: `SourceSeaTunnelTask#isUnboundedSourceTask` (`SourceSeaTunnelTask.java:172`), rejects missing flow/action/source. - F2: `CheckpointCoordinatorTest#testRunningRestoreCompletesCheckpointWithoutFinishedReaderAcknowledgements` (`CheckpointCoordinatorTest.java:88`) and `CheckpointCoordinatorFailoverIT#testCheckpointCompletesAfterMasterFailoverWithIdleClosedReader`. - F3 / F5: `SubPlan#isCheckpointCoordinatorAlreadyStarted` Javadoc (`SubPlan.java:662`) states that every coordinator must be RUNNING and that a finished enumerator is out of scope; `PhysicalVertex#isRecoverableFinishedStateForRunningPipelineRestore` Javadoc (`PhysicalVertex.java:361`) states the state-only limit. Rationale for the state-only decision is in the PR description. - F4: `SubPlan#isRecoverableRunningState` reads `getExecutionState()` once and passes it down (`SubPlan.java:680`). - F7: `allMatch` in `isCheckpointCoordinatorAlreadyStarted` (`SubPlan.java:670`); single loop over `taskGroup.getTasks()` in `PhysicalVertex` (`PhysicalVertex.java:374`). - F8: `PhysicalVertex` Javadoc (`PhysicalVertex.java:361`). Local verification is limited to `spotless:apply` / `spotless:check` on the engine-server module per the active policy; runtime validation is the current-head GitHub CI run. -- 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]
