SEZ9 commented on PR #11856: URL: https://github.com/apache/seatunnel/pull/11856#issuecomment-5852104617
Thanks for the follow-up — head is still `dc2c64103c`, so I'm treating everything below as pending the next push. **F1** - Comment in `markLocalGracefulMemberRemoval()` (`SeaTunnelServer.java`): sounds right. Please make sure it spells out the dependency on `LifecycleServiceImpl.shutdown()` firing `SHUTTING_DOWN` before distributed-object proxies stop being served, and the Hazelcast version (5.1) it was verified against. - Per-method result: agreed that the class-level `Tests run: 10, Failures: 0, Errors: 0, Skipped: 2` from run `35240527669` isn't enough on its own. Once the new run is in, please paste the surefire line for `testGracefulShutdownPublishesMemberRemovalMarker` here so we can confirm it actually executed and isn't one of the two skips. F1 stays open until then. **F2** - The (a) walk-through — non-destructive `get` at `CoordinatorService.java:2185`, value-conditional `remove` at `:2207` only after the propagation loop (`:2222-2238`) and only when `canClearGracefulMemberRemovalMarker` allows it (`:2239-2245`) — addresses the "destructively consumed before processing" concern for the event that reads the marker. Thanks for restating it in full. - (b) failover: the point that the new master re-derives the classification via `PhysicalVertex.checkTaskGroupIsExecuting` (`PhysicalVertex.java:278-288`) without clearing is a reasonable answer. Please add a short comment at that read site noting it is intentionally non-clearing and why, so the two read paths don't drift apart later. - The `restoringRunningJobsFromMasterSwitch` reset bug you found is exactly the kind of thing that turns the explicit clear into dead code and pushes every removal onto the TTL — glad you're treating it as blocking. For the fix, please: (1) reset the flag when the restore actually completes (including the failure/early-exit paths of the restore, not just the happy path), and (2) add a test that performs a master switch with active jobs and then verifies a subsequent graceful removal clears the marker immediately rather than waiting out the TTL. That test is what will let us close F2. **Remaining asks for the next push** 1. F1 code comment + per-method surefire line for `testGracefulShutdownPublishesMemberRemovalMarker`. 2. F2 flag-reset fix + master-switch regression test, plus the non-clearing comment on the `PhysicalVertex` read. 3. A status line on F3–F8 (throwable vs. string payload, marker eviction/TTL, message-based classification, docs, and the duplicated offline template) — even a "not yet addressed" is fine, I just want them tracked alongside F1/F2 rather than falling off. Ping when the push is up and I'll re-review against the new head. <!-- streview-comment:1354 --> -- 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]
