SEZ9 commented on PR #12688:
URL: https://github.com/apache/seatunnel/pull/12688#issuecomment-6104581159
Thanks @DanielLeens — and thank you for going back and checking `updatedAt`
yourself. No apology needed: I took that suggestion and shipped it without
verifying how the engine sets the field, so the dead rule was mine to have
caught. It is a good reminder that "use the server's timestamp" is only safe
once you know *when* the server writes it.
All three items are fixed at head `7063bacf`, and the carried-over items are
fixed in #12631 (now at `d196f01b`) with this branch rebased onto it.
## 1. `updatedAt` is last-event time, so the stalled rule could never fire
(blocking) — fixed
Confirmed from source before changing it.
`setUpdatedAt(System.currentTimeMillis())` appears in exactly one place in the
tree: inside the `overviewMap.compute` callback of
`HazelcastCheckpointOverviewStateStore.updateOverview`, which
`CheckpointMonitorService` calls on trigger, acknowledge, complete, fail and
restore. So for a checkpoint hung on its last subtask the newest write is that
last acknowledgement, nothing follows, and `updatedAt - triggerTimestamp`
freezes a few seconds after the trigger. The error-level rule was unreachable
in the one situation it exists for, and the `first_round_overdue` gate with it.
The `updatedAt` branch is gone. `run_diagnose` now resolves the engine's
clock once from `diagnostics.generatedAt` on the `/job-info` response — which
`JobRuntimeDiagnostics.build` stamps at response time, as you noted — and
passes it into `diagnose_checkpoints`, with the local clock as the fallback
inside each rule set. Both halves of the diagnosis therefore do their
arithmetic in the same clock as the timestamps they subtract from, which was
the point of the original suggestion and now actually holds.
The docstring records why `updatedAt` is *not* used, so the next person does
not re-make the same inference from the field name.
Tests replaced:
`test_a_stuck_checkpoint_is_detected_even_though_updated_at_is_stale` uses the
realistic shape you described — `updatedAt` three seconds after the trigger,
the trigger itself well past the threshold relative to the injected now — and
asserts the stalled finding still appears.
`test_without_an_injected_clock_the_local_one_is_used_not_updated_at` passes no
`now_ms` at all, so it fails if anything starts consulting `updatedAt` again.
## 2. Monitor-off 200 response gets no notice — fixed
Confirmed: `CheckpointMonitorRestService.getOverview` builds `{"jobId":
...}` and returns it before adding `updatedAt`/`pipelines` when
`getMonitorService()` is null or the overview is absent. No exception, so the
`except`-only notice never ran.
The notice is now driven by whether the response is *usable* — a dict with a
`pipelines` list — rather than by whether the request raised:
```
Job 852 my-job — RUNNING
No checkpoint history for this job, so the slowdown checks did not run
(the cluster may predate the endpoint, its monitor service may be off, or
the job may not have checkpointed yet).
```
Your "fresh job" wording suggestion is in there, since that case is
indistinguishable from the monitor being off at this level.
Two decisions in it worth flagging, both of which a test forced me to make
explicit:
- The notice is gated on a non-terminal job status, through the same
`is_non_terminal()` helper #12631 uses for the `diagnostics` notice. A finished
or failed job legitimately has no checkpoint overview retained, so without the
gate this fired on every terminal job.
`test_a_missing_checkpoint_endpoint_does_not_break_the_diagnosis` now asserts
the notice is *absent* there, with the live-job case covered separately.
- "Nothing to report" is suppressed whenever a notice was printed. The two
together would contradict each other, and the notice is the more truthful of
the pair.
Tests: `test_a_cluster_with_the_monitor_off_is_reported_not_passed_over`
uses `200 {"jobId": "852"}` exactly as you asked, and
`test_a_healthy_job_with_no_checkpoint_data_does_not_look_fully_checked` now
asserts that "Nothing to report" does *not* appear.
## 3. Stalled threshold above the default checkpoint timeout — documented
Verified: packaged `config/seatunnel.yaml` sets `checkpoint.timeout: 60000`
against `STALLED_CHECKPOINT_SECONDS = 120`. The comment above the constant now
says that on default settings a hung checkpoint expires first and surfaces as
`checkpoint_failing`, so the rule earns its keep on clusters that have raised
the timeout. Same note added to the rule list in both READMEs, so a user on
defaults knows where to look.
## Diff and CI
This branch is cut fresh from `dev` now that #12629 has merged, so the
replayed commit is gone; the diff is 8 files, **+3203/-0**, and still carries
#12631's commit until that lands, as the description says. Merge order
unchanged: #12631 first, then this.
Locally: 327 tests pass, `ruff check .` clean with the pinned
`ruff==0.16.7`. The engine-side facts above are from source reading; I did not
run a cluster.
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]
