DanielLeens commented on PR #11708:
URL: https://github.com/apache/seatunnel/pull/11708#issuecomment-5379306717
# What Problem Does This PR Solve?
- User pain point: the original `job.retry.times` doc only said "the default
value is 3," leaving it ambiguous whether the retry counter is
per-failure-episode or cumulative for the pipeline's whole life, and whether a
long healthy run "buys back" retry budget.
- Fix approach: this PR's own commit (`6f49d488c1ac`) added one explanatory
paragraph to `docs/en` and `docs/zh` stating the counter is per-pipeline,
cumulative, and never reset by a successful recovery.
- One-sentence summary: the underlying behavioral claim is correct and
verified against the current engine source, but the branch itself is now an
empty diff — the same content, with a more accurate caveat this PR's own
wording lacked, already landed on `dev` through PR #11748, and nothing remains
here to merge.
# 1. Code Change Review
## 1.1 Core Logic Analysis
**Status of this round.** Head is still
`e3f0c2176ae5bf1b855cdda65db5ef9d572d408a` (unchanged since 2026-08-14). No new
commit and no new comment/review from anyone since my last check on 2026-08-21.
I re-verified, rather than assumed, that this remains true:
- `gh api repos/apache/seatunnel/pulls/11708` at this moment reports
`changed_files: 0`, `additions: 0`, `deletions: 0` — same as every check since
2026-08-16.
- `git diff` between the PR head and current `dev` tip for
`docs/en/introduction/configuration/JobEnvConfig.md` and
`docs/zh/introduction/configuration/JobEnvConfig.md` is empty — both files
remain byte-identical to `dev`, which has moved further since my last check
(now `behind_by=48` vs. `28` on 2026-08-19) with no further edits to this
section.
- `dev`'s current text at
`docs/en/introduction/configuration/JobEnvConfig.md` (immediately after the
`job.retry.times` default-value sentence) still reads: "This counter
accumulates for the life of the pipeline; an intermediate successful recovery
does not reset it. For example, with `job.retry.times = 5` ... The one
exception is an active-master failover in a Zeta cluster, which rebuilds the
pipeline execution plan (and its retry counter) from scratch." This is the
#11748 wording, and it is strictly more accurate than this PR's own original
sentence ("resubmitting the job ... is the only way to reset the counter"),
which omitted the master-failover reset path — the exact gap I originally
flagged as Issue 1 on 2026-08-10.
- `mergeable: true`, `mergeable_state: blocked` — the `REVIEW_REQUIRED`
gate, not a conflict, consistent with there being nothing left in the diff to
conflict with.
I am not re-deriving the underlying `SubPlan.pipelineRestoreNum` /
`CoordinatorService.restoreJobFromMasterActiveSwitch` call chain a third time
in full — I traced it end to end (declaration, sole increment site, `reset()`
deliberately not touching it, the monotonic gate, and the two
`JobMaster.init()` call sites) in my 2026-08-16 review, which is still visible
on this PR and whose file:line citations I re-spot-checked this round against
current `dev` source without finding drift.
## 1.2 Compatibility Impact
**Fully compatible / not applicable** (unchanged) — the diff is empty, so
trivially no API, config, checkpoint, or serialization surface is touched.
## 1.3 Performance / Side-Effect Analysis
None — no code or runtime path is touched.
## 1.4 Error Handling and Logging
Not applicable — no code changed.
# 2. Code Quality Assessment
## 2.1 Coding Standards
Not applicable — docs-only, and the diff is currently empty.
## 2.2 Test Coverage and Test Stability
No tests changed, correctly so for a docs PR. As noted in my 2026-08-16
review, the underlying documented behavior ("no reset on an intermediate
successful recovery," plus the master-failover exception) still has no direct
test coverage of its own — `CheckpointErrorRestoreEndTest.java` and
`JobClientJobProxyIT.java` only exercise continuous-failure paths. This is a
pre-existing gap in `seatunnel-engine`, not something this PR should be asked
to fix (Issue 2, Low, informational).
## 2.3 Documentation Updates
The content this PR set out to add is fully present on `dev` (via #11748),
in both `docs/en` and `docs/zh`, in a form that is more accurate than this PR's
own original wording. There is nothing left for this PR itself to contribute.
# 3. Architectural Soundness
## 3.1 Elegance of the Solution
The original intent was well-judged — document real,
previously-undocumented, verified behavior in the exact place a user
configuring `job.retry.times` will read it. The PR's current state, however, is
not a solution at all: its conflict-resolution merge on 2026-08-14 correctly
resolved everything in favor of `dev`, leaving an empty branch.
## 3.2 Maintainability / 3.3 Extensibility
Not applicable to an empty diff.
## 3.4 Historical-Version Compatibility
Not applicable — documentation only, no behavior, upgrade, or serialization
surface touched, and the documented behavior itself is long-standing and
unchanged.
# 4. Issue Summary
| # | Issue | Location | Severity | Status |
|---|---|---|---|---|
| 1 | PR is an empty diff (0 files / 0 additions / 0 deletions); the content
it set out to add is already on `dev` via #11748, in a more accurate form
(includes the active-master-failover reset caveat this PR's own wording
omitted) | whole PR; intended targets
`docs/en/introduction/configuration/JobEnvConfig.md:48`,
`docs/zh/introduction/configuration/JobEnvConfig.md:48` | Medium | Still true,
re-verified this round (`dev` has moved 20 commits further since my last check
with no further edits to this section) |
| 2 | The documented "no reset on an intermediate successful recovery"
behavior, and the master-failover exception, have no direct test coverage in
`seatunnel-engine-server` | `CheckpointErrorRestoreEndTest.java:38-64`,
`JobClientJobProxyIT.java:52-64` | Low | Pre-existing gap, out of this PR's
scope |
# 5. Merge Recommendation
### Conclusion: Not recommended for merge — close as superseded by #11748
1. **Blockers**
- Issue 1 (Medium): there is nothing to merge. The correct disposition is
to close this PR as superseded, not to merge an empty commit and not to push a
commit re-adding the original (less accurate) paragraph.
2. **Recommended (non-blocking)**
- Issue 2 (Low): a follow-up unit test in `seatunnel-engine-server`
pinning "no reset after a successful intermediate recovery" plus the
master-failover exception would be valuable, but belongs in a separate PR
against `seatunnel-engine`, not here.
**Overall assessment.** No reviewer other than myself has commented on this
PR, so there is no other position to reconcile. The technical question this PR
raised was worth asking and the answer is correct and re-verified again this
round against current `dev` source: `job.retry.times` is a per-pipeline,
cumulative, monotonic counter that survives successful recoveries, with the one
real reset trigger beyond resubmission being a Zeta active-master failover.
That outcome is fully and more accurately achieved on `dev` today through
#11748. CI is green at this head. Since this is purely a bookkeeping item with
zero correctness or compatibility risk, I'd suggest the author (myself) close
this PR as superseded rather than leaving an empty-diff branch open for future
reviewer attention.
--
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]
