SEZ9 commented on PR #12688:
URL: https://github.com/apache/seatunnel/pull/12688#issuecomment-6096279316
Thanks @DanielLeens — all five items plus the rebase are addressed in
`bcb55b4ab`, and the inherited #12631 items are fixed in the parent (now at
`d426a0b87`), which this branch is rebased onto.
Your framing was right and it generalised further than the two rules you
named: both blocking findings here are the same mistake as issues 2 and 3 in
#12631 — a counter that accumulates over the job's lifetime, read as a
statement about the present. `counts` is the worst offender because *every*
value in it is cumulative.
## 1. `checkpoint_failing` on a lifetime counter (Medium, blocking) — fixed
The rule now asks "did the latest attempt fail", not "has anything ever
failed", using the comparison you suggested:
- `latestFailed.triggerTimestamp` newer than
`latestCompleted.triggerTimestamp` → warning, now worded "and its latest
checkpoint failed rather than completed"
- a failure on record with nothing ever completed → warning (the latest
failure is the only outcome there is)
- the latest completion is newer → `info` `checkpoint_failed_before`: "has N
failed checkpoints since it was submitted, but the most recent checkpoint
completed, so it is committing progress now"
- neither timestamp usable → `info` with the count and the engine's reason,
and an explicit "Whether the latest one failed is not visible in this response"
`completed` is in the evidence string now, as you asked: `failed=4
completed=36`.
That last branch is a change from your suggestion worth flagging: an empty
`latestFailed` object is a shape the engine really sends, and the existing
`test_a_failed_count_still_reports_when_the_reason_is_unusable` pins that the
measured count must not be discarded just because the explanation is missing.
So the count still surfaces — as a note, with no claim about the present.
Tests: `test_old_failures_under_a_newer_completion_are_not_a_warning` (the
5000-completion pipeline carrying one expired checkpoint from 20 days ago —
your exact scenario),
`test_a_failure_newer_than_the_last_completion_is_a_warning`,
`test_failures_with_no_completion_on_record_are_a_warning`.
## 2. `checkpoint_never_completed` on a fresh job (Medium, blocking) — fixed
It now needs one of the two gates you proposed: `triggered >=
MIN_TRIGGERS_NEVER_COMPLETED` (2), or an in-progress checkpoint that has been
in flight longer than `STALLED_CHECKPOINT_SECONDS`. `triggered=1, completed=0,
inProgress=1` three seconds in — the reading every healthy job has in its first
interval — is now silent.
When the age gate is what fired, the evidence says so: `triggered=1
completed=0 oldest in progress for 2m10s`.
Tests:
`test_a_job_in_its_first_checkpoint_interval_is_not_reported_as_never_completing`,
`test_a_first_checkpoint_that_is_already_overdue_is_reported`,
`test_a_second_trigger_with_nothing_completed_is_reported_without_timestamps`.
## 3. Markup in the non-finding lines (Medium, blocking) — fixed in the
parent
Moved to #12631 as you asked, so this PR no longer carries that fix alone.
There, `rich.markup.escape()` covers the header, the 404 line, the HTTP-error
body and the transport-error line, and the test that asserted only the
uppercase `API-02` now asserts `ErrorDescription:[master is down]`. Details are
in my reply on #12631, including the `MarkupError` reproduction for a bracketed
path.
## 4. The silent skip (Low) — fixed
A failed checkpoint read now prints one line and logs the exception at debug:
```
Checkpoint history unavailable, so the slowdown checks did not run
(the cluster may predate the endpoint, or its monitor service is off).
```
You were right that the dangerous case is the *combination*: no job-level
finding and no checkpoint read, where the output used to end at "Nothing to
report" and read as "the slowdown checks ran and found nothing".
`test_a_healthy_job_with_no_checkpoint_data_does_not_look_fully_checked` pins
exactly that pair, and
`test_a_missing_checkpoint_endpoint_does_not_break_the_diagnosis` now also
asserts the notice.
## 5. Local clock vs engine timestamps (Low) — fixed
`updatedAt` is now the clock when `now_ms` is not injected, falling back to
the local clock when the overview does not carry it.
`test_the_overview_updated_at_is_preferred_over_the_local_clock` passes no
`now_ms` at all, so the rule has to find its clock in the response.
For symmetry, the job rules in the parent now use `diagnostics.generatedAt`
the same way.
## Inherited items and the rebase
All the #12631 carry-overs are fixed in the parent, including the default
base URL — I verified from `JettyService`, `RestConstant` and the packaged
`config/seatunnel.yaml` that port 5801 cannot serve these paths, and the
default is now 8080. That verification is static, from source and docs; I did
not run a packaged distribution, so the empirical confirmation you asked for is
still open. Full detail, including a scope note about changing a pre-existing
default, is in my #12631 reply.
#12629 has merged. The conflict was the single `cli.py` import hunk; the two
READMEs auto-merged rather than conflicting.
## Verification
- 312 tests pass locally, `ruff check .` clean with the `ruff==0.16.7` that
`cli.yml` pins.
- CI green on this head: `Build`, `Lint`, `Test with Python 3.10`, `Test
with Python 3.13`.
- Wire-format and engine behaviour claims are read from source; nothing was
run against a live cluster.
One housekeeping note, same as on the parent: because #12629 was
squash-merged under a new SHA, this branch's merge base predates it and GitHub
still replays `rest.py` and `tests/test_rest.py` into the diff. The content is
byte-identical to `dev`, so it merges cleanly — I can rebase to drop it if a
smaller diff would help the re-review. This PR will still carry #12631's commit
until that one lands, as the description says.
Ready for another look.
--
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]
