SEZ9 commented on PR #11856: URL: https://github.com/apache/seatunnel/pull/11856#issuecomment-6553000321
Thanks for merging `dev` (`3ac2d1cec6a`) into this branch as `408723b36e2`. The conflict resolution as described sounds reasonable: taking `dev`'s `exec java` launch path and dropping `testClusterScriptExecsForegroundServer` in favour of `dev`'s stricter test, and combining the two `PhysicalVertexTest.java` classes into one. I'll wait for the fork CI run (41165429981) for compile/test status. This update covers the merge itself; it doesn't say whether any of the previously raised findings changed between `eed3d9f2f4` and `408723b36e2`. Before I re-review, could you state, per finding, whether it was addressed or is still open: - **PR11856-F1 (HIGH)** – publishing the graceful-shutdown marker from `ManagedService.shutdown()`, where the node is already PASSIVE and IMap writes can be rejected. This is the blocker: if the marker write silently fails, the WARN-downgrade no-ops in real graceful shutdowns. Has the marker publication been moved earlier in the shutdown sequence, or is there a test showing the marker is reliably present after a graceful shutdown? - **PR11856-F2** – the marker is destructively consumed before task-failure processing and has no physical expiry; master failover mid-processing loses the graceful classification. - **PR11856-F3 / PR11856-F6** – the graceful path replaces the `JobException` throwable in `TaskExecutionState` with a plain String, which contradicts the "state handling unchanged" claim and changes the error payload users see in job history / REST. - **PR11856-F4** – `engine_gracefulMemberRemoval` entries have no eviction and the manual TTL check depends on cross-node clock agreement. - **PR11856-F5** – classifying log level by free-text message match means any failure that mimics the offline template gets downgraded to WARN. - **PR11856-F7** – you mention both state-storage-and-recovery docs are part of this PR; do those changes describe the new marker IMap and the WARN downgrade of node-offline failures? - **PR11856-F8** – the offline-message template exists twice (format string in `CoordinatorService.java`, regex in `PhysicalVertex.java`) and will drift silently. If any of these are intentionally deferred, please say so explicitly so we can decide whether they block this PR or can be follow-ups. For F1 in particular I'd like to see it resolved (or shown not to be a problem) before merge. <!-- streview-comment:2192 --> -- 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]
