davidzollo commented on PR #12027: URL: https://github.com/apache/seatunnel/pull/12027#issuecomment-5551957247
@DanielLeens Thanks for the thorough re-review. Both points from your last round are now closed out. ### Issue 1 (High, blocking) — resolved by evidence, not by argument The run you saw as `queued` at review time has since completed, and it is green on the exact head you reviewed (`e9923e5337c`): - `apache/seatunnel` `Build` check → `conclusion: success`, `head_sha: e9923e5337c`, completed `2026-09-05T12:39:34Z` - fork run [`33961795042`](https://github.com/davidzollo/seatunnel/actions/runs/33961795042) (same head) → `conclusion: success` - `Run / engine-v2-it (8, ubuntu-latest)` → **success** - `Run / engine-v2-it (11, ubuntu-latest)` → **success** That is the first green result on this test after three consecutive identical failures (`73a8c0da70f`, `e8fc49e1662`/`db8889317`, `bcf9ad180`), and it passed on both JDKs. It confirms the diagnosis: the hang was the holder job's static worker slots never being reclaimed after `cancelJob()` + immediate master shutdown — a separate resource-lifecycle defect — not the epoch-scheduling invariant this test exists to guard. Removing the cancel-and-wait step in favour of growing the cluster is what made the difference. Agreed on your tracking note as well: this CI evidence lines up with the existing #11437 / #9589 slot-leak reports rather than warranting a third issue, so I'll attach it there instead of opening a new one. ### Issues 2 and 3 — fixed in `222b8bba` - **Issue 2 (Medium):** added the class-level Javadoc, covering the shared setup (real split cluster, master ownership changes while worker slots are occupied) and all four scenarios in the class, including `testTerminalJobCleanupSkipsWorkerWaitAfterMasterSwitch` that landed independently from #12034. - **Issue 3 (Low):** you were right, `testPendingJobLifecycleAcrossMasterFailover` exists nowhere in the repo — the only occurrence of that string was the comment itself. Corrected to `testPendingJobLifecycleInMasterFailover`, and I verified it is the right precedent: that test starts a second worker instance after the failover to release its pending job, which is the same pattern the comment describes. `222b8bba` is **comment-only** — no test logic, no assertion, no helper touched (+23/−1, all Javadoc/comment text; `git diff e9923e53..222b8bba` shows nothing else). Two useful consequences: 1. It cannot regress the green result above. 2. Because the test logic is byte-identical to the head that just passed, the CI run this push triggers is an independent repeat of exactly the same test — which gives you the second confirming run you asked for as part of addressing the nits, rather than as a separate cycle. I'll report back once that run completes. Verified locally with `./mvnw spotless:apply` on `connector-seatunnel-e2e-base` (clean, no reformatting beyond the change itself); per this repo's workflow all compile/test validation is left to CI. -- 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]
