SEZ9 commented on PR #11856: URL: https://github.com/apache/seatunnel/pull/11856#issuecomment-5707770751
Thanks for the rebase. `fd60dfd596` is a pure `dev` merge on top of `e6e243744e` with content identical to the previously reviewed head, so the earlier findings still apply. As noted above, the "Ready to merge" conclusion posted at `e6e243744e` is retracted: green CI alone was not sufficient evidence, and the engine E2E logs show the WARN downgrade has not fired on the graceful-shutdown path. **Blocking** - **PR11856-F1 — marker published too late.** `SeaTunnelServer.onShutdown()` runs from the `GracefulShutdownAwareService` hook, after the node has already moved to `PASSIVE`, so the `engine_gracefulMemberRemoval` write is rejected and the coordinator never sees a marker. Please move the marker write to a point where the node is still `ACTIVE` (for example the synchronous `SHUTTING_DOWN` lifecycle event) and add an E2E assertion for the WARN-level offline log line so this cannot regress silently. **Should be addressed before merge (medium)** - **PR11856-F3 / PR11856-F6 — payload change on the graceful path.** `CoordinatorService` replaces the `JobException` throwable in `TaskExecutionState` with a plain string, so job history / REST error fields lose the stack trace. That contradicts the "failure state handling unchanged" statement in the PR body. Please keep the throwable and only narrow the log level. - **PR11856-F2 / PR11856-F4 — marker lifecycle.** The marker is destructively consumed before task-failure processing (a master failover mid-processing loses the graceful classification), and the IMap has no eviction; the TTL is only a manual timestamp check that depends on cross-node clocks agreeing. Please consume after processing (or make it idempotent) and configure a real per-entry TTL on the map. - **PR11856-F5 / PR11856-F8 — message-based classification.** The offline template exists as a format string in `CoordinatorService` and as a regex in `PhysicalVertex`; any free-text failure that happens to match gets downgraded, and the two definitions can drift independently. A typed marker (dedicated exception subclass or an explicit flag on the state) would address both. - **PR11856-F7 — docs.** The new `engine_gracefulMemberRemoval` IMap and the WARN downgrade are cluster-visible behavior; please add a short note under `docs/`. Once F1 is fixed, could you paste the engine E2E log excerpt showing the marker write succeeding and the offline failure logged at WARN? That is the evidence missing at `e6e243744e`, and it would be good to have in the thread before re-approving. <!-- streview-comment:1113 --> -- 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]
