DanielLeens commented on PR #10551:
URL: https://github.com/apache/seatunnel/pull/10551#issuecomment-5569358750
Thanks for flagging both points, @SEZ9.
1. On the "cut off at Sorry for" concern — I just re-pulled the raw review
body via the API (review id 5125052007) and it is not actually truncated on my
end: the text runs continuously from "Sorry for missing this in my 2026-08-18
review..." through the rest of the `validateJobSubmissionFence` correction and
into the "Having now actually reviewed it..." analysis, with nothing missing.
That looks like a rendering/collapse artifact on the web UI rather than a
truncated post. If a hard refresh still shows it cut off for you, let me know
and I'll paste the missing section directly into a comment.
2. Your mapping is correct — F1/F3/F4-F6/F5/F7/F8 line up 1:1 with Issues
4/2/3/2/5/6 from my last full review, and none of them have moved on this head:
the only production change since 2026-09-01 was the single
`getOwnedPendingCleanup(jobId, jobInfo)` substitution at
`CoordinatorService.java:1217` (Issue 1, now closed). So there's nothing to
point you at yet — those items are still exactly as I left them, same
files/lines:
- F1 -> `repairMissingJobStateForRestore`'s jobId-only fence,
`CoordinatorService.java:1254`
- F3/F5 -> lock-ordering + `jobMaster.init(...)` under the distributed
lock, `CoordinatorService.java:1208-1230, 987-1005`
- F4/F6 -> `cleanupPendingJobStateMaps` abort-on-first-failure,
`CoordinatorService.java:987-1005, 884-926`
- F7 -> zombie `JobInfo` when repair fails with no owned cleanup record,
`CoordinatorService.java:1174-1181`
- F8 -> missing foreign-generation-during-restore regression test in
`CoordinatorServiceJobCleanupTest.java`
I called these non-blocking recommended follow-ups rather than hard blockers
since none is a live correctness bug in the currently committed code, but
holding them as a bar for your own approval is a reasonable, stricter stance
given this PR's history — happy to re-verify each one the moment a commit lands
addressing them.
Separate factual update since my last pass: the required `Build` check,
which was still `in_progress` when I posted, has since completed and is now
`FAILURE` on this same head (`805608cd8aa1`). Worth the author's attention
before we get to a source-clear state for merge.
--
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]
