SEZ9 commented on PR #10551: URL: https://github.com/apache/seatunnel/pull/10551#issuecomment-6552520437
Thanks for the continued work on this one. A quick status update on the current head `61cd8a20a87e52cf42ec2701ba4b91128bd92e67`: **Merge state / CI** - GitHub now reports a real content conflict against the latest base. Please rebase onto the current base, resolving the conflict so that both this PR's change and the intervening upstream work are preserved, then push the resulting commit so required checks can run against it. - On the existing head, [Signal pull request update](https://github.com/apache/seatunnel/actions/runs/40632030941/job/122630383647) is `SUCCESS` but [Build](https://github.com/apache/seatunnel/runs/122630450067) is `FAILURE`. Could you take a look at the build failure and either fix it or share the root cause if it looks unrelated to this change? **Open review items (still blocking)** The restore/cleanup fencing concerns from the previous review in `CoordinatorService.java` remain open. To recap what is still needed: 1. **Generation/ownership check on the cleanup fence (F1, F2):** `repairMissingJobStateForRestore` and the restore fence both key off `pendingJobCleanupIMap` by `jobId` alone. A stale cleanup record from an older generation of the same `jobId` will block state repair / abort restore of the currently active generation, and the job never comes back after a master switch. The fence needs to validate that the record actually belongs to the generation being cleaned up before it short-circuits restore. 2. **Lock ordering (F3):** the new `pendingJobCleanupIMap.lock(jobId)` → `runningJobStateIMap.lock(key)` nesting is introduced in two places. Please document the ordering contract and confirm it is also respected in `DistributedStateTransition`, otherwise a cluster-wide lock-order deadlock is possible. 3. **`jobMaster.init(...)` under the distributed lock (F5):** please move the init out of the `pendingJobCleanupIMap` lock scope in the master-switch restore path, or explain why holding it across init is safe. 4. **Cleanup robustness (F4, F6):** `cleanupPendingJobStateMaps` stops at the first `remove` failure with no retry/reschedule, and the scheduler catch only logs. This leaves partial old-generation residue (and, after `runningJobInfoIMap.remove`, orphaned state entries) until the next master switch. Either make the cleanup idempotent and re-schedulable, or continue past individual failures and retry. 5. **Zombie JobInfo (F7):** when state repair returns `null` and no owned cleanup record exists, restore currently leaves the `JobInfo` in place permanently, where the old code removed the stale entry. Please restore that removal (or an equivalent path) for this case. 6. **Test coverage (F8):** please add a test for the unowned stale-cleanup-record-during-restore scenario so the fence behaviour above is pinned down. Once the rebase is pushed with the build green and the items above addressed (or discussed if you disagree with any of them), I'll do another pass. <!-- streview-comment:2183 --> -- 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]
