DanielLeens commented on PR #11495:
URL: https://github.com/apache/seatunnel/pull/11495#issuecomment-6266721701
Thanks @SEZ9. To save you a trip through the diff, here are the pointers on
head `9c047310570519504bd5d37be03421b46b377dba` (read from source only; I have
not run a failover reproduction):
- **F1 (stale task-group contexts)**: `SubPlan.prepareRestorePipeline` calls
`jobMaster.clearPipelineMetricsForRestore(getPipelineLocation())` at
`SubPlan.java:497`. In `JobMaster.java`, `clearPipelineMetricsForRestore` (line
1287) first calls `cleanTaskGroupContext(pipelineLocation)` (line 1289, defined
at 1310), which sends `CleanTaskGroupContextOperation` to the still-alive
owners, and only then calls `seaTunnelServer.removeMetrics(pipelineLocation)`
(line 1303).
- **F2 (race)**: it is documented, not closed. The window is an in-flight
report, plus the case where the clean fails on a worker that is still alive. I
asked @zhangshenghang above to state that case explicitly in the PR description
so the PR does not over-claim a full fix for #11472. I agree with you that this
is best-effort by design.
- **F3 (counters restart from zero)**: the note is in
`docs/en/introduction/concepts/incompatible-changes.md` and the `docs/zh` twin,
headed "Behavior change: runtime counters no longer accumulate values from a
failed pipeline attempt", with a shorter note in
`docs/{en,zh}/engines/zeta/rest-api-job-lifecycle.md`.
- **F4 (tests)**: in `StateTransitionCleanupTest`,
`testRestoreClearRunsTaskGroupCleanupBeforeMetricsRemoval` covers the order,
`testRestoreClearRemovesMetricsFromTheStateStore` uses the real
`MetricsSnapshotStateStore` and checks other pipelines are untouched, and
`testMetricsCleanupFailureDoesNotPreventRestore` covers the failure path. No
test simulates a late report from the old attempt, so that case is not covered.
One new item for @zhangshenghang: GitHub currently reports this PR as
`DIRTY` (merge conflicts with `dev`; the branch is `ahead_by=8, behind_by=49`).
Please sync the latest `dev` and resolve the conflicts. `Build` is green on the
current head, but it should be re-confirmed after the sync.
--
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]
