SEZ9 commented on PR #10551: URL: https://github.com/apache/seatunnel/pull/10551#issuecomment-5594755050
Thanks @DanielLeens, that answers my question on the F2 hunk — the `pendingJobCleanupIMap.get(jobId)` → `getOwnedPendingCleanup(jobId, jobInfo)` swap in the second `pendingJobCleanupIMap`-fenced block of `restoreJobFromMasterActiveSwitch` is exactly the spot I was asking about. I'll pull `805608cd8aa16dc4865f136ae53578efac2acffc` and check the full patch. Provided `getOwnedPendingCleanup` actually validates ownership/generation against `jobInfo` (rather than just null-checking), I'm happy to consider F2 addressed. Two things on that commit before I sign off on F2: 1. Since the only production change is that one line, F1 is still open — `repairMissingJobStateForRestore` still uses the jobId-only `containsKey` fence as far as I can tell from your description. Please confirm whether that path is also switched to the ownership-aware lookup, or whether it's intended for a separate commit. 2. On the two rewritten `CoordinatorServiceJobCleanupTest` cases: do either of them exercise the *unowned* stale-record-during-restore case (older generation record present, current generation restores successfully)? If they only cover the owned-record path, F8 remains open and I'd like a dedicated test for the unowned scenario. Also, note the hunk sits right before `jobMaster.init(...)` while the distributed lock is still held, so F5 is unchanged by this commit — just flagging so it doesn't get lost. Your comment appears to have been cut off at "On splitting any of them into a follow-up PR" — could you finish that thought? My current position: F1/F2/F8 should land in this PR since they're the core correctness/test gap for the new fences. For F3/F4/F5/F6/F7 I'm open to a follow-up PR if you propose which ones and why, but I'd want at least the lock-ordering contract (F3) documented here, as it's introduced by this change. Bar for approval stays F1/F3/F4/F5/F6/F7/F8, with F2 pending my read of the commit. <!-- streview-comment:900 --> -- 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]
