DanielLeens commented on PR #11856: URL: https://github.com/apache/seatunnel/pull/11856#issuecomment-5846260494
Hi @SEZ9, thanks for the clear breakdown of what's still open. Head is still `dc2c64103c`, unchanged since my last comment, so nothing below required re-checking the source — just closing the gaps you flagged. **F1** - Code comment: agreed, and it isn't landed yet. I'll add it to `markLocalGracefulMemberRemoval()` in `SeaTunnelServer.java` in the next push, stating that correctness depends on `LifecycleServiceImpl.shutdown()` firing `SHUTTING_DOWN` before distributed-object proxies stop being served, and naming the Hazelcast version (5.1) this was verified against. - Per-method result: still don't have it, and I won't claim more than I have. All I've confirmed is the class-level `Tests run: 10, Failures: 0, Errors: 0, Skipped: 2` for `ClusterFaultToleranceIT` from run `35240527669`. The next push (which also carries the fix below) will get a fresh CI run — I'll pull the per-method surefire line for `testGracefulShutdownPublishesMemberRemovalMarker` from that run and confirm it isn't one of the two skips before calling F1 closed. **F2** — restating in full since it cut off again: `CoordinatorService.java:2185` does a non-destructive `IMap.get`. The value-conditional `remove` at `:2207` only runs after the propagation loop finishes (`:2222-2238`), and only when `canClearGracefulMemberRemovalMarker` allows it (`:2239-2245`). So for the event that reads the marker, it is not removed before that event's own task-failure processing completes — that's (a). For (b), master failover between read and clear: the new master doesn't depend on the old master's in-flight clear at all. It re-derives classification independently during restore, through `PhysicalVertex.checkTaskGroupIsExecuting`, which reads the same marker without clearing it (`PhysicalVertex.java:278-288`). So a failover mid-processing does not lose the graceful classification by itself. What it does expose is a real bug I found on this same pass: `restoringRunningJobsFromMasterSwitch` is only reset inside `initCoordinatorService()`, never after a restore actually completes. So after the first master-switch restore with active jobs, `canClearGracefulMemberRemovalMarker` never returns `true` again for the rest of that master's term — the explicit post-processing clear becomes dead code, and every later removal falls back to waiting out the 5-minute TTL instead of being cleared immediately. That's the one thing I'm treating as blocking right now, and it's going into the same next push as the F1 comment, with a regression test that drives two failovers and asserts the clear fires both times. **F3–F8** — fair to ask again, and I owe you F4 specifically: I skipped it in my last comment without flagging that I was skipping it. That one wasn't a rendering issue on your end, just an omission on mine — apologies for adding to the confusion given the genuine cut-offs elsewhere in this thread. Full status, nothing changed on any of these since `dc2c64103c`: - F3/F6 (JobException vs. String payload): resolved. `buildMemberRemovedFailureState` wraps the message in `JobException` on both branches (`CoordinatorService.java:2144-2149`), pinned by `shouldKeepThrowablePayloadForMemberRemovedFailureState`. - F4 (eviction / clock dependency): resolved. Eviction is a Hazelcast-side per-entry TTL on the `put` itself (`SeaTunnelServer.java:309-313`, `Constant.GRACEFUL_MEMBER_REMOVAL_MARK_TTL_MILLIS` = 5 minutes) — the entry expires on its own whether or not anything ever reads it. `isGracefulMemberRemovalMarkerValid` (`CoordinatorService.java:2157-2161`) is a separate, secondary check on top of that: it compares the departing member's timestamp against the coordinator's clock and rejects anything more than 5 minutes skewed. That part does depend on clocks roughly agreeing, but it fails in the safe direction — a marker that fails this check just falls back to `ERROR`, it never gets treated as graceful. - F5/F8 (message-pattern matching / duplicated template): resolved. No `Pattern` or `.matches(` anywhere in `PhysicalVertex.java`; classification is a typed boolean threaded through from the marker lookup, and the offline message string has exactly one producer, `buildMemberRemovedOfflineMessage` (`CoordinatorService.java:2133-2137`). - F7 (docs): resolved. Both `docs/en/engines/zeta/state-storage-and-recovery.md` and the `docs/zh` counterpart carry the marker/TTL/WARN-vs-ERROR section. So, directly answering "next push vs. defer": everything above except the `restoringRunningJobsFromMasterSwitch` reset bug and the version-dependency comment is already landed, and I'm not touching it again. Those two go into the next commit together, along with the regression test and the per-method CI evidence for F1. I'll follow up on this thread once that's pushed rather than asking you to re-derive status from a comment again. One more thing on the rendering pattern itself, since this is now the third time a comment in this thread has shown up cut off on your side (the interleaving explanation in August, the F4 explanation on the 20th, and my last comment on the 24th), while the stored body has checked out complete via the API every time I've verified it: I don't have visibility into what's causing that on your end, so I can't fix it directly, but I'll keep future updates shorter and split across separate comments where practical, since that seems like the more robust workaround until the underlying cause is understood. -- 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]
