DanielLeens commented on PR #11856: URL: https://github.com/apache/seatunnel/pull/11856#issuecomment-5812765027
Hi @SEZ9, thanks for going through this so carefully again. On the rendering issue: I already flagged this once for the F4 explanation at the top of my last review (`dc2c64103c`, review id 5289961888), and it looks like it's happened again, this time apparently cutting off partway through section 1.1. I just re-pulled that review body directly through the GitHub API to double-check: it is **not** actually truncated server-side — the stored comment is about 22.6 KB / 186 lines and continues well past the "TTL/clock-skew rule" sentence, through the Key findings, the runtime-path trace, the full Issue 1 write-up, sections 1.2-1.4, all four numbered issues, sections 2-5, and the merge recommendation. So this looks like a GitHub UI/client rendering artifact on long comments rather than a real content gap on my end. Restating the parts you couldn't see, point by point: **Issue 4 (the item behind F1's second ask)** — already logged as a formal issue in that review, not something missing: > `markLocalGracefulMemberRemoval()` should document its Hazelcast-version dependency. Location: `SeaTunnelServer.java:280-290`. The correctness of the whole feature depends on `LifecycleServiceImpl.shutdown()` firing `SHUTTING_DOWN` before the node stops being able to serve distributed-object proxies — established only by reading the Hazelcast 5.1 source, not asserted anywhere near this method. Severity: Low. Raised by another reviewer: Yes (you, comment 5747479000, F1 item 2). To be precise about status: this is a recommended fix from the review, not yet landed in the diff — `dc2c64103c` hasn't changed since my 09-18 comment, so no comment has been added near that method yet. It's tracked as non-blocking (Low), same tier as Issue 2 (sync IMap calls / log noise) and Issue 3 (TTL-vs-heartbeat docs gap). **F1, per-method test confirmation** — still genuinely open; I'd rather say that plainly than restate the class-level number as if it settles it. What I have is `Tests run: 10, Failures: 0, Errors: 0, Skipped: 2` for the whole `ClusterFaultToleranceIT` class from fork run `35240527669`. I have not found a per-method line for `testGracefulShutdownPublishesMemberRemovalMarker` specifically in that log — I said the same in my 09-19 comment and don't have a stronger source today. Honest status: class-level pass confirmed, method-level still unconfirmed. **F2, non-destructive read / clear timing** — unchanged from my 09-19 answer and still correct: `CoordinatorService.java:2185` does a non-destructive `IMap.get`, and the value-conditional `remove` at `:2207` only runs after the propagation loop finishes (`:2222-2238`) and only when `canClearGracefulMemberRemovalMarker` allows it (`:2239-2245`). So the marker is not removed before task-failure processing completes for the event that read it. What my latest pass adds on top is Issue 1: `restoringRunningJobsFromMasterSwitch` is reset only inside `initCoordinatorService()` and 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 clear effectively becomes dead code and everything falls back to the 5-minute TTL. That's a real, currently open gap (High), distinct from the non-destructive-read question, which is fine on its own. **F3/F6, F5/F8, F7** — unchanged from the 09-19 table, nothing new: the `JobException` payload shape is still pinned by a dedicated test (F3/F6, resolved); there's no `Pattern`/`.matches(` string-matching left in `PhysicalVertex.java` and classification is a single typed boolean from one producer helper (F5/F8, resolved); both `docs/en` and `docs/zh` `state-storage-and-recovery.md` carry the new marker/TTL/WARN-ERROR section (F7, resolved). Net status: the only thing blocking merge from my side right now is Issue 1 (the never-reset flag) — a small, well-scoped fix (reset it once the restore future completes) plus a regression test. Issues 2-4 are non-blocking follow-ups. No new commit has landed on `dc2c64103c` since my 09-18/09-23 passes, so there's nothing else on this head to re-verify yet. -- 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]
