SEZ9 commented on PR #12631:
URL: https://github.com/apache/seatunnel/pull/12631#issuecomment-6104580780
Thanks @DanielLeens — and no need to apologise for issue 1; you found it
while re-tracing the servlet, which is exactly where it was hiding. Both
blockers plus items 4 to 6 are fixed at head `d196f01b`. Item 3 is now its own
PR, as you suggested.
I verified each of your claims against the source before changing anything
rather than taking them from the review, since one of last round's items turned
out to need correcting. All of them checked out.
## 1. Unknown job id answered with HTTP 200 (blocking) — fixed
Confirmed: `JobInfoService.getJobInfoJson` falls through to `new
JsonObject().add(JOB_ID, jobId.toString())` when the id is in neither the
running-job map nor the finished-job state. HTTP 200, no `jobStatus`.
The fix keys on the status rather than on the HTTP code: a response whose
`jobStatus` is not a non-empty string is reported as an unknown id and exits 1.
That also covers a non-object body — `null`, a list, a bare string from a proxy
— before anything calls `.get()` on it, which is the second half of your item 4.
The 404 branch stays for engines and proxies that do send one. Test fixtures
now use the real shape (`200 {"jobId": "..."}`), with
`test_a_real_404_is_still_handled` keeping the other branch covered and
`test_a_non_object_json_body_does_not_crash` covering the malformed bodies.
For the second case you raised — a RUNNING job whose response carries no
`diagnostics` — the output now says so explicitly instead of ending on "Nothing
to report":
```
Job 852 my-job — RUNNING
Restart counts and state ages were not available in this response, so the
crash-loop and stuck-state checks did not run (the master may not be
serving diagnostics for this job).
```
One refinement I had to make after a test caught me: I first printed that
for any job without `diagnostics`, which fired on every `FINISHED` job, because
the engine builds the block only for a job the master still coordinates. So the
notice is now gated on a non-terminal status, via a small `is_non_terminal()`
helper in `jobs.py` that the caller and the stuck rule share. And "Nothing to
report" is suppressed whenever a notice was printed, so the two never
contradict each other.
## 2. End-state pipelines reported as restarting (blocking) — fixed
Confirmed from `PipelineStatus.isEndState()` (`FINISHED || CANCELED ||
FAILED`) and from `SubPlan.prepareRestorePipeline`, which increments
`pipelineRestoreNum` and then immediately `reset()`s the pipeline back toward
`CREATED` before the restore interval. So a pipeline *observed* in an end state
is not mid-restart — it is done, and the count is history. Exactly the
cumulative-counter mistake from last round, in a place I had missed.
The rule now dispatches on the pipeline's own lifecycle rather than on
`status != "RUNNING"`:
| pipeline status | reported as |
|---|---|
| `FINISHED`, `CANCELED` | nothing — it is over |
| `FAILED` | only when exhausted, past tense: "used all N of its restores
and then failed" |
| `CANCELING`, `FAILING` | `info` — restores are history, and the stop may
have been requested |
| `CREATED`, `SCHEDULED`, `DEPLOYING`, `INITIALIZING` | `warning` — between
attempts, so not processing data |
| `RUNNING` | the recency logic from last round |
| missing / unparsable | `info` with the bare count |
Two deliberate choices worth flagging. First, the end-state check runs
**before** the exhausted branch, so a `FINISHED` pipeline at its restore limit
no longer produces an error about a "next failure" that cannot happen — you
called that gap out and it needed the ordering, not just a skip. Second, I
extended the grouping to `CANCELING`/`FAILING` even though you did not ask:
they hit the same `status != "RUNNING"` branch, so a user-requested stop was
also being reported as "restarting again". It seemed better to fix the whole
branch once than to leave the same false claim in a neighbouring state.
Tests: `test_finished_and_cancelled_pipelines_are_not_called_restarting`,
`test_an_end_state_pipeline_at_its_restore_limit_is_not_an_error_about_the_future`,
`test_a_failed_pipeline_is_reported_in_the_past_tense_only_when_exhausted`,
`test_a_tearing_down_pipeline_reports_restores_as_history`,
`test_a_pipeline_between_attempts_is_reported_as_such`, and
`test_a_finished_pipeline_next_to_a_running_one_is_not_reported` for the
multi-pipeline case you named.
## 3. Default-port change (suggested split) — split out
Agreed, and done: **#12722**, with its own issue **#12721**. It carries the
one-line default, the two README columns, `env.example.sh` (your item 5) and a
test that also pins that the environment override still wins. Its description
states the `/run` effect you identified, with the confirmation-panel output,
and notes it for the release note.
This PR no longer touches `connectors.py` or `env.example.sh` at all. The
README text here now says to point `SEATUNNEL_API_BASE` at the Jetty port
rather than asserting what the default is, so the two PRs can land in either
order without one making the other's docs wrong.
## 4-6. The low items — done
- **4.** The HTTP error body now goes through `redact_credentials` and a
2000-character clip that says how much was cut;
`test_the_http_error_body_is_redacted_and_capped` checks a password does not
reach the terminal and that the output stays small. The non-object JSON guard
is folded into issue 1's check.
- **5.** In #12722 with the rest of the port change.
- **6.** Reworded to the two facts and a pointer, as you suggested: "has
restarted N times, and the current attempt is only X old. Check the task logs
for what triggered the restores." The README rule list now also records that a
loop whose attempts each survive longer than ten minutes is reported as
history, and documents the end-state handling from issue 2.
## Diff and CI
The replayed #12629 commit is gone — this branch is cut fresh from `dev` now
that #12629 has merged, so the diff is 6 files, **+1812/-0**, just this feature.
Locally: 291 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]
