SEZ9 commented on PR #12631: URL: https://github.com/apache/seatunnel/pull/12631#issuecomment-6096278077
Thanks @DanielLeens — all eight items are addressed in `d426a0b87`. The principle behind issues 2 and 3 is the useful part of this review and I applied it beyond the two rules you named: **a rule may only assert what a single response can prove.** Every blocking finding here turned out to be the same mistake in a different place — a counter that accumulates over the job's lifetime, read as a statement about the present. ## 1. Rich markup (Medium, blocking) — fixed, and moved down the stack as you asked `markup=False` now applies to the finding lines in *this* PR rather than in #12688, and `rich.markup.escape()` covers the header, the 404 line, the HTTP-error body and the transport-error line. Both the job name and the engine body are caller-supplied text, so neither can be trusted as markup. I reproduced all three symptoms with the same theme before fixing them, rather than taking them on faith: | string passed to `console.print` | printed before the fix | |---|---| | ` [warning] Pipeline 1 has restarted 5 times (restoreCount=5/10)` | `Pipeline 1 has restarted 5 times (restoreCount=5/10)` — label gone | | ` [error] ErrorCode:[API-02], ErrorDescription:[master is down]` | `ErrorCode:[API-02], ErrorDescription:` | | ` [error] cannot write [/data/x.csv]` | `MarkupError: closing tag '[/data/x.csv]' at position 23 doesn't match any open tag` | New tests, each of which fails on the old behaviour: `test_the_severity_label_reaches_the_terminal`, `test_bracketed_engine_text_is_not_swallowed`, `test_a_path_that_looks_like_a_closing_tag_does_not_crash`, `test_a_bracketed_job_name_is_not_read_as_markup`. And `test_other_http_errors_show_the_engine_body` now asserts `ErrorDescription:[master is down]` rather than the uppercase `API-02` — your diagnosis of *why* this slipped through was exactly right, so the assertion that permitted it is gone. ## 2. Cumulative `restoreCount` read as a rate (Medium, blocking) — fixed `pipeline_crash_loop` now requires evidence that the loop is current, taken from the pipeline's own `stateTimestamps` and `pipelineStatus`: - pipeline not `RUNNING` → warning (it is mid-restart, so the loop is observable) - `RUNNING` with the last entry into `RUNNING` younger than `CRASH_LOOP_RECENT_SECONDS = 600` → warning, and the text now says how old the attempt is - `RUNNING` for longer than that → `info` `pipeline_restored_before`: "has restarted N times since it was submitted, most recently X ago, and has been running since". No "making no progress" claim, because that would be false - no usable status or timestamp → `info` with the bare count and "How recently is not visible in this response" The restore-budget error is untouched, as you suggested: reaching `maxRestoreCount` needs no timing to be true. Tests: `test_repeated_restores_with_a_young_attempt_are_reported_as_a_crash_loop`, `test_old_restores_on_a_steady_pipeline_are_history_not_a_crash_loop`, `test_a_pipeline_not_running_with_restores_behind_it_is_a_crash_loop`, `test_restores_with_no_timing_evidence_are_reported_as_a_bare_count`. ## 3. Progress rules with no age gate (Medium, blocking) — fixed Both row-count rules now require the job to have been `RUNNING` for at least `STUCK_SECONDS`, measured from `stateTimestamps["RUNNING"]`. One deliberate detail: this reads the `RUNNING` key directly instead of going through `_state_entered_at`, whose fallback to the newest entry is right for the stuck rule but wrong here — an unknown uptime has to mean silence, not a guess, since the entire finding is the difference between "has read nothing for an hour" and "started two seconds ago". For `job.mode = STREAMING` the idle case is now `info`, not `warning`, worded so it does not pretend to know which it is: a quiet topic and a misconfigured one produce an identical reading. Batch keeps the warning, where reading nothing is a fault by definition. Tests: `test_a_freshly_running_job_that_has_read_nothing_yet_is_not_flagged`, `test_progress_rules_stay_silent_without_a_running_timestamp`, `test_an_idle_streaming_source_is_a_note_not_a_warning`, `test_a_batch_source_reading_nothing_is_still_a_warning`. ## 4. The default base URL (Medium, blocking until verified) — you were right `_ENGINE_API_BASE` now defaults to `http://localhost:8080`. What I checked: - `JettyService` registers **every** endpoint this CLI calls as a servlet on the Jetty HTTP port — `/running-jobs`, `/option-rules`, `/submit-job`, `/job-info`, `/jobs/checkpoints`. - 8080 is that port both as the code default and in the packaged `config/seatunnel.yaml` (`enable-http: true`, `port: 8080`). - On 5801 the same paths are served only by `RestHttpGetCommandProcessor` under `RestConstant.CONTEXT_PATH = "/hazelcast/rest/maps"`, and only when Hazelcast's own REST API is switched on, which the packaged config does not do. So the old default 404s on every call, not only on `/job-info`. **How far I verified it, precisely:** this is static verification from the engine source, the packaged config and `rest-api-v2.md`. I did **not** run it against a packaged distribution, so the empirical check you asked for is still open — if anyone with a running cluster can confirm, that would close it properly. **Scope, since this is bigger than the PR's own feature:** you offered "document port 8080, or fix the default in a separate PR". I took the third path on purpose — shipping `--diagnose` with a default that 404s makes the feature broken out of the box, and the misleading 404 you flagged would be the normal experience. The change is one line plus its comment, `test_the_default_api_base_is_the_port_that_serves_job_info`, and the README default column. **If you would rather keep this PR narrow, say so and I will split it into its own PR** — it is a clean lift. The 404 message now also names `SEATUNNEL_API_BASE` and the port distinction, so a user who does hit it gets pointed at the cause instead of being told their job id is wrong. ## 5-7. The low-severity items — all three done - **5.** `diagnostics.generatedAt` is now the clock when `now_ms` is not injected, with the local clock as fallback for an engine that does not send it. Every timestamp being compared was written by the master, so the arithmetic now stays inside one clock. `test_the_masters_generated_at_is_preferred_over_the_local_clock`, `test_a_missing_generated_at_falls_back_to_the_local_clock`. - **6.** `--diagnose` validates `-?\d+` before building the URL; `1/2` is rejected with "is not a job id" and `test_a_non_numeric_job_id_is_rejected_before_any_request` asserts no request is made at all. - **7.** `DOING_SAVEPOINT` has its own bucket and rule (`savepoint_in_progress`): it reports the duration and says a large state legitimately takes this long, instead of claiming a task is ignoring the stop request. `test_a_long_savepoint_is_not_described_as_an_unresponsive_task`. ## 8. Rebase — done #12629 has since merged, and the conflict was the single `cli.py` import hunk you predicted; resolved by keeping both imports. The README conflicts you expected did not materialise — those two files auto-merged. ## Verification - 278 tests pass locally, and `ruff check .` is clean with `ruff==0.16.7`, the version `cli.yml` pins, so this is the same result the Lint job produces. - CI on this head is green: `Build`, `Lint`, `Test with Python 3.10`, `Test with Python 3.13`. - The engine-side facts above are read from source; nothing here was run against a live cluster. ## Two notes that are not findings **The same markup bug is live in `dev` right now.** You flagged it as latent in the `Root cause:` line added by #12628, and it is worse than latent — I reproduced it: ``` cli.py:1439 self.console.print(f" Root cause: {parsed.headline()}", style="info") ``` With `errorMsg` of `java.io.IOException: cannot write [/data/x.csv]`, `headline()` returns that text verbatim and the print raises `MarkupError`. That crashes the repair flow on a job failure whose message contains a bracketed path. It is merged code and outside this PR, so I will send it as its own small fix unless you would rather it rode along here. **The diff still shows #12629's files.** #12629 was squash-merged under a new SHA, so this branch's merge base still predates it and GitHub replays `rest.py` (+106), `tests/test_rest.py` (+146) and the `agents.py` cleanup into this PR's diff. The content is byte-identical to what is in `dev` (I compared per file), so it merges cleanly and changes nothing functionally — but it makes you re-read ~250 lines of already-merged code. Happy to rebase and drop that commit if it would make the re-review easier; I left the head stable for now so your comments keep pointing at live lines. Ready for another look when you have time. -- 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]
