SEZ9 commented on PR #10551: URL: https://github.com/apache/seatunnel/pull/10551#issuecomment-5578088499
Thanks @DanielLeens — appreciate the detailed answers on both points. 1. Thanks for checking the raw body of review 5125052007. That's good enough for me to treat it as a rendering/collapse artifact rather than a truncated post; if it still looks cut off after a refresh I'll ask for the section, but no need to re-paste it now. 2. Thanks for confirming the mapping. On the `getOwnedPendingCleanup(jobId, jobInfo)` substitution at `CoordinatorService.java:1217` (F2): I'd like to verify that against the diff on `805608cd8aa1` before I mark it resolved on my side — could you point me at the commit or hunk that introduced it? I understand you're treating the remaining items as non-blocking follow-ups; I'm going to keep them as the bar for my own approval given this PR's history, so to be explicit about what I still need: - F1 — replace the jobId-only `pendingJobCleanupIMap.containsKey` fence in `repairMissingJobStateForRestore` (`CoordinatorService.java:1254`) with the same ownership/generation-aware check used at line 1217, so a stale record from an older generation can't permanently block repair of the active generation. - F3 / F5 — document the `pendingJobCleanupIMap.lock(jobId)` → `runningJobStateIMap.lock(key)` ordering contract (and confirm `DistributedStateTransition` honours it), and move `jobMaster.init(...)` out from under the distributed lock in the master-switch restore path (`CoordinatorService.java:1208-1230, 987-1005`). - F4 / F6 — make `cleanupPendingJobStateMaps` (`CoordinatorService.java:987-1005, 884-926`) either continue past a failed remove or reschedule, so a partial failure doesn't leave orphaned state and a live cleanup record until the next master switch. - F7 — when repair returns null and there is no owned cleanup record (`CoordinatorService.java:1174-1181`), remove the stale `JobInfo` as the old code did rather than leaving a zombie entry. - F8 — add the foreign-generation-during-restore regression test in `CoordinatorServiceJobCleanupTest.java` covering the unowned stale-cleanup-record path. Once a commit lands for these I'll re-review promptly. If you'd prefer to split any of them into a follow-up PR, please say which ones so we can agree on that up front. <!-- streview-comment:879 --> -- 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]
