DanielLeens commented on PR #11495:
URL: https://github.com/apache/seatunnel/pull/11495#issuecomment-6210064113
Thanks @SEZ9 for the follow-up. To save a round trip, here is where each of
your three points stands on `9c047310570519504bd5d37be03421b46b377dba`, as I
read the code (I have not run a failover reproduction):
1. Worker cleanup and the re-report race. `SubPlan.prepareRestorePipeline`
(`SubPlan.java:497`) now calls `JobMaster.clearPipelineMetricsForRestore`,
which first sends `CleanTaskGroupContextOperation` to every owned slot whose
worker is still a cluster member (per task group, failures logged and skipped),
and only then removes the pipeline snapshots from the master store. If the
clean fails on a worker that is still alive, that context is not removed, so it
can still re-publish the old attempt's counters after the wipe. Both the
Javadoc and the PR notes describe the race as a millisecond-scale window for an
in-flight report, but neither states this failed-clean-on-a-live-worker case
explicitly. I agree with you that it should be stated plainly so the PR does
not over-claim a complete fix for #11472. @zhangshenghang, could you add one
sentence for it to the PR description?
2. Counter semantics. The behavior change is listed in
`docs/en/introduction/concepts/incompatible-changes.md` (and the `zh` twin)
under "Zeta: runtime metric counters restart from zero after a pipeline
failover restore", with a shorter note in
`docs/{en,zh}/engines/zeta/rest-api-job-lifecycle.md`. It covers streaming jobs
resumed after a failover. The PR description's Notes section also points to
both places.
3. Tests. `testRestoreClearRunsTaskGroupCleanupBeforeMetricsRemoval` uses
`InOrder` to prove clean-before-remove.
`testRestoreClearRemovesMetricsFromTheStateStore` seeds the real
`MetricsSnapshotStateStore` and asserts the restored pipeline's snapshots are
gone while other pipelines are untouched. None of the tests simulates a late
worker report from the old attempt, so the "late report does not reappear"
assertion you describe is not covered. Given the best-effort design, I would
treat that as a nice-to-have rather than a blocker, but I defer to you on
whether you want it before approving.
CI: the `Build` check on this head is green now. I'll re-check as soon as
the description update lands.
--
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]
