DanielLeens commented on PR #11856:
URL: https://github.com/apache/seatunnel/pull/11856#issuecomment-5342958510
Thanks for working on this! I re-traced the whole call chain independently
against the current head (`51702fa2b`) before reading back through the thread,
so let me fold my pass into the review already posted above rather than
duplicate it — the conclusion is the same, and I want to add two things that
review flagged as open: extra source-level confirmation for Issue 1, and the CI
root cause (the earlier pass noted "logs were not retrievable at review time").
# Confirmation of Issue 1 (Critical) — the regex is unreachable on the real
path
Tracing `errorByPhysicalVertex` end to end:
```
CoordinatorService.makeTasksFailed()
[CoordinatorService.java:2006-2013]
-> new TaskExecutionState(loc, FAILED, new JobException(String.format(
"The taskGroup(%s) deployed node(%s) offline", taskGroupLocation,
lostAddress)))
-> TaskExecutionState(TaskGroupLocation, ExecutionState, Throwable)
[TaskExecutionState.java:32-40]
throwableMsg = ExceptionUtils.getMessage(throwable)
-> ExceptionUtils.getMessage(Throwable)
[ExceptionUtils.java:28-42]
calls throwable.printStackTrace(pw) and returns the full dump, NOT
throwable.getMessage()
```
`JobException` (`org.apache.seatunnel.engine.common.exception.JobException`)
has no `toString()` override, so `Throwable.toString()` falls back to
`getClass().getName() + ": " + getMessage()`. That means the first line
`printStackTrace()` emits is literally:
```
org.apache.seatunnel.engine.common.exception.JobException: The
taskGroup(...) deployed node(...) offline
at
org.apache.seatunnel.engine.server.CoordinatorService.lambda$makeTasksFailed$...(CoordinatorService.java:2011)
at java.util.ArrayList.forEach(ArrayList.java:...)
...
```
`DEPLOYED_NODE_OFFLINE_ERROR_PATTERN` (`PhysicalVertex.java:81-82`) is
anchored `^The taskGroup\(.+\) deployed node\(.+\) offline$` and checked with
`.matches()` — a whole-string match, no `DOTALL`/`MULTILINE`. The class-name
prefix alone fails the `^The taskGroup` anchor, independent of the multi-line
issue. So `isDeployedNodeOfflineFailure()` returns `false` for every real
invocation, `stateProcess()`'s `FAILED` branch always falls through to the
`else`, and `log.error` still runs unconditionally — this PR ships with zero
behavior change on its only production call path. `makeTaskGroupFailing()`
(`PhysicalVertex.java:654-656`) has the identical `ExceptionUtils.getMessage()`
write pattern, so there's no alternate producer that would store a bare message
either.
The new `PhysicalVertexTest` only calls the static predicate with a
hand-written clean string, so it can't catch this — it never goes through
`TaskExecutionState(..., Throwable)` the way `CoordinatorService` actually does.
I'll also add one adjacent observation on scope, since it affects how Issue
2 should be fixed: `SubPlan.addPhysicalVertexCallBack` (`SubPlan.java:209-219`)
already logs `log.error("Task %s Failed in %s, Begin to cancel other tasks in
this pipeline.", ...)` unconditionally for **every** `FAILED` task,
node-offline or not, before the pipeline decides whether to auto-restore from
checkpoint (`SubPlan.java:737-744`, gated by
`checkNeedRestore`/`pipelineMaxRestoreNum`). So even once Issues 1 and 2 are
fixed, the pipeline-level ERROR signal that "a task failed" will still fire for
node-offline events — operators/alerting scraping for ERROR won't lose
visibility that *something* failed. What they will lose is the node-address
detail this PR's WARN message carries, since that detail only exists in the
(currently dead) `PhysicalVertex`-level log line. Worth keeping in mind when
deciding whether to also downgrade or restructure the `SubPlan`-level log once
Issue 2's crash/scale-down disti
nction is designed.
# CI diagnosis (root cause now confirmed, both unrelated to this diff)
Fork run
[`32158223580`](https://github.com/DanielLeens/seatunnel/actions/runs/32158223580)
for this exact head (`51702fa2b454ce02b38409db7caec76ee0e1aa90`) has 2 failing
jobs (not 3 — `all-connectors-it-7` isn't part of this run's job list at all):
- **`unit-test (11, windows-latest)`** — [job
log](https://github.com/DanielLeens/seatunnel/actions/runs/32158223580/job/95784074576):
`JobStateEventTest>AbstractSeaTunnelServerTest.before:70 » IllegalState Node
failed to start`. This is the known Windows/Hazelcast node-bootstrap flake seen
repeatedly on `windows-latest` runners across unrelated PRs — environmental,
not connected to `seatunnel-engine-server`'s `dag/physical` package this PR
touches.
- **`engine-v2-it (11, ubuntu-latest)`** — [job
log](https://github.com/DanielLeens/seatunnel/actions/runs/32158223580/job/95784074646):
`ClusterFaultToleranceTwoPipelineIT.testTwoPipelineStreamJobRestoreIn2NodeMasterDown`
fails with `SeaTunnelEngineRetryableException: Can not get coordinator service
from an active master node.`, thrown from `SeaTunnelServer.java:313` — a
bounded-retry timing window during master failover election in a 2-node
kill-the-master test (202s test, 17s to this specific failure). This exception
path is in `SeaTunnelServer`/master-election, not anywhere this PR's diff
touches (`PhysicalVertex.stateProcess()`'s log-level branch and the new regex
helper), so it reads as pre-existing CI timing flakiness for this specific IT
rather than something introduced here.
Neither failure is caused by this diff. They should still be
re-run/confirmed green (or explicitly triaged as known flakes) before merge,
but they are not evidence against the change itself — the blocking issues are
the ones above.
# Merge Recommendation (unchanged)
### Conclusion: Not recommended for merge
Blockers (carried over from the review above, now with added evidence):
1. Issue 1 (Critical) — confirmed via
`JobException`/`ExceptionUtils.getMessage()` source: the WARN downgrade never
fires in production; needs a structural signal (typed exception / flag on
`TaskExecutionState`) set at `CoordinatorService` and consumed as-is, not
regex-matching a rendered stack trace.
2. Issue 2 (High) — the trigger (`MembershipServiceEvent` member-removed)
doesn't distinguish graceful scale-down from crash/OOM-kill/network-partition,
so a correct fix for Issue 1 would also silence genuine outages at WARN unless
scope is narrowed.
3. CI — both current failures are environmental/timing flakes unrelated to
this diff (see above); not a code blocker, but should be green (or explicitly
waived as known flake) before merge.
No blocking issue with the new test's stability or license/formatting —
those are fine as-is.
--
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]
