SEZ9 commented on PR #10551: URL: https://github.com/apache/seatunnel/pull/10551#issuecomment-5611811333
Thanks @DanielLeens — appreciated on both points. On the "truncated" comment: understood, that was a rendering/collapse artifact on my side, not a real cut. Sorry for the noise. On the head mismatch: you're right, I was working from `805608cd8aa1`. I hadn't picked up `f6919c586496` yet, and I have not verified that commit's diff myself, so I'm going off your description below until I re-check. **F1** — if `repairMissingJobStateForRestore`'s fence in `f6919c586496` now goes through `getOwnedPendingCleanup(jobId, jobInfo) != null` instead of the jobId-only `containsKey` check, that is exactly the fix F1 was asking for. The regression test you describe (`testMissingStateRepairIgnoresStaleCleanupRecordFromForeignGeneration`, stale record at `initializationTimestamp=100` vs. registered `JobInfo` at `200`, asserting `JobStatus.CREATED` and that the stale record is cleared) also covers the scenario in the finding. I'll re-read against that head and, assuming it matches what you describe, mark F1 resolved. **F8** — agreed with your split. The two rewritten `CoordinatorServiceJobCleanupTest` cases in `805608cd8aa1` (`testSubmitStartWithSavePointConsumesOwnedPendingCleanupAndSucceeds` and the `SavepointDoneState` variant) exercise the *owned* record path only, so they don't close F8 for the `submitJob` / `validateJobSubmissionFence` savepoint-restart fence. The new test in `f6919c586496` covers the foreign-generation case for the *repair* fence, which is F1's territory. So F8 stays open for the submit-side fence specifically. Concrete remaining ask for F8: one test that seeds an **unowned** (foreign-generation) `JobCleanupRecord` in `pendingJobCleanupIMap` for the same jobId, then goes through the `submitJob` savepoint-restart path and asserts the submission is not aborted by that stale record. Same shape as the F1 regression test, just targeting the submit fence. Your comment cuts off mid-sentence at "covers the foreign-generation case" — if there was more after that (e.g. anything on the other findings), could you repost the tail? I don't want to presume the rest. <!-- streview-comment:935 --> -- 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]
