DanielLeens commented on PR #11708:
URL: https://github.com/apache/seatunnel/pull/11708#issuecomment-5340767722
# Re-verification Update (2026-08-19) — no new commit or reply since my
2026-08-16 review
I re-ran this as a full fresh pass rather than skipping it for lack of new
activity, since this is my own PR and I want to make sure the conclusion still
reflects the current `dev` state rather than going stale.
**Facts re-checked at this head:**
- PR head is still `e3f0c2176ae5bf1b855cdda65db5ef9d572d408a` — unchanged
since 2026-08-16, no new commits, no new comments from anyone.
- `gh api .../pulls/11708/files` still reports `changed_files: 0`,
`additions: 0`, `deletions: 0`.
- `git diff origin/dev HEAD --
docs/en/introduction/configuration/JobEnvConfig.md
docs/zh/introduction/configuration/JobEnvConfig.md` against the *current* `dev`
tip (`2586f37e`, as of 2026-08-19) is still completely empty — the two files
remain byte-identical to `dev`.
- `dev`'s `JobEnvConfig.md:48` still carries the accurate clarification from
#11748: cumulative counter, no reset on intermediate successful recovery, and
explicitly calls out the active-master-failover exception.
- I re-traced the underlying source again against current `dev` to make sure
nothing drifted since 08-16: `SubPlan.java:102/129` (`pipelineRestoreNum` is
still a plain in-memory `AtomicInteger`, initialized only in the constructor),
`SubPlan.java:492` (still the sole `getAndIncrement()`), `SubPlan.java:425-435`
(`reset()`, invoked on every restore attempt, still does not touch the field),
and `CoordinatorService.java:1010/1056` (`restoreJobFromMasterActiveSwitch` →
`jobMaster.init(..., true)` still rebuilds the `SubPlan` graph, still the one
non-resubmission reset path). All of it is unchanged and confirms the same
conclusion as before: the underlying behavioral claim this PR set out to
document (`job.retry.times` is a cumulative, per-pipeline counter that survives
successful recoveries and is only reset by resubmission or a master
active-switch) is **true**, and it is **already fully and more accurately
documented on `dev` via #11748**.
- `compare/dev...HEAD`: `status=diverged`, `ahead_by=2`, `behind_by=28`.
`mergeable=true`, `mergeable_state=blocked` (this reflects the
`REVIEW_REQUIRED` gate, not a merge conflict — there is nothing left in the
diff to conflict with). CI is green: `Build` / `Notify test workflow` /
`labeler` all pass.
Nothing here changes my 2026-08-16 conclusion, so I'm not asking for
anything new — just confirming it's still accurate against the latest source
rather than leaving a stale-looking review sitting on the PR.
# Merge Recommendation
### Conclusion: Not recommended for merge — close as superseded by #11748
The technical question this PR raised was worth asking, and the answer is
correct and verified end-to-end against the current engine source:
`job.retry.times` really is a per-pipeline, cumulative, monotonic counter
(`SubPlan.pipelineRestoreNum`) that is never reset by an intermediate
successful recovery, and the one real reset trigger beyond resubmission is a
Zeta active-master failover rebuilding the plan
(`CoordinatorService.restoreJobFromMasterActiveSwitch` → `JobMaster.init(...,
true)`). That's exactly what's now documented on `dev` at `JobEnvConfig.md:48`
via #11748 — with wording that's strictly more accurate than this PR's own
original sentence, which incorrectly claimed resubmission was "the only way" to
reset the counter.
This branch's own conflict-resolution merge already resolved everything in
favor of `dev`, which was the right call on the merits, but it also means the
PR is now an empty diff (0 files changed) with nothing left to merge. Landing
it as-is would only add a no-op merge commit to `dev` history. There's no
code-side blocker and no compatibility/behavioral risk — this is purely a
housekeeping item.
1. **Blockers — must be fixed**
- None from a content/correctness standpoint. The only reason this PR
can't be merged is that it has no remaining diff.
2. **Recommended fixes — non-blocking**
- Close this PR as superseded by #11748 rather than pushing a commit to
re-add the (less accurate) original paragraph. If any residual value is wanted,
the only real follow-up is a dedicated unit test pinning "no reset on
intermediate successful recovery" plus the master-failover exception in
`seatunnel-engine-server` — that belongs in a separate PR against
`seatunnel-engine`, not here.
Since the goal this PR set out to achieve is already live on `dev`, closing
it as superseded is the appropriate next step here rather than keeping an
empty-diff PR open.
--
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]
