SEZ9 commented on PR #10551:
URL: https://github.com/apache/seatunnel/pull/10551#issuecomment-5564545223
Thanks @DanielLeens for the from-scratch re-review of `805608cd8aa1` and for
diffing it byte-for-byte against `7f21b1a3a315` — that confirms the only
production change is the `pendingJobCleanupIMap.get(jobId)` →
`getOwnedPendingCleanup(jobId, jobInfo)` substitution in the second lock-fenced
block before `jobMaster.init(...)`, and I agree that closes the
unowned-stale-record restore abort (PR10551-F2). The reentrancy note on
`IMap#lock` for the same jobId inside `getOwnedPendingCleanup` matches my
reading as well.
Two things on my side:
1. Your comment appears to be cut off at "Sorry for" — the carryover
correction about the 2026-08-18 "zero diff" claim didn't come through. Could
you re-post the rest? I'd like to know whether it changes the status of
anything below.
2. Since this round touched exactly one line, the remaining items from the
earlier scope are, as far as I can tell, still open on this head. Concretely,
what I still need before I can approve:
- **PR10551-F1**: `repairMissingJobStateForRestore` still uses a
jobId-only `pendingJobCleanupIMap.containsKey` fence. The same
`getOwnedPendingCleanup(jobId, jobInfo)` ownership check should be applied
there, otherwise a stale older-generation record still blocks repair for the
active generation.
- **PR10551-F3**: please document the `pendingJobCleanupIMap.lock(jobId)`
→ `runningJobStateIMap.lock(key)` ordering contract in the code (javadoc on
both call sites is fine) and confirm `DistributedStateTransition` never
acquires them in the reverse order.
- **PR10551-F4 / PR10551-F6**: `cleanupPendingJobStateMaps` still aborts
on the first `remove` failure and the scheduler catch only logs. Either
continue past individual failures and only drop the cleanup record once every
removal succeeded, or reschedule the cleanup on failure — I'm fine with either
as long as the record isn't left permanently live with partial residue.
- **PR10551-F5**: `jobMaster.init(...)` is still executed while holding
the distributed `pendingJobCleanupIMap` lock. If narrowing the lock to the
fence-check + state-repair is not feasible, please say why in a comment so the
next reader knows it's intentional.
- **PR10551-F7**: when repair returns null and there is no owned cleanup
record, please restore the old behavior of removing the stale `JobInfo` (or
explain why leaving it is now safe).
- **PR10551-F8**: a test for the unowned
stale-cleanup-record-during-restore path — now that `getOwnedPendingCleanup` is
in the restore fence, this is the case that would have caught the original bug.
If any of these were already addressed in a way I'm not seeing on this head,
point me at the spot and I'll re-check.
<!-- streview-comment:862 -->
--
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]
