DanielLeens commented on PR #11458:
URL: https://github.com/apache/seatunnel/pull/11458#issuecomment-5601168454
Thanks @SEZ9.
**On the "cut off" text**: it wasn't actually truncated in the underlying
comment — I re-pulled the raw review body via the API just now and the full
sentence is there ("Carried-over, non-blocking items from my last full review:
- Issue 2 ... - Issue 3 ..."). That was most likely GitHub's "show more"
collapse on a long review body, not a real cut-off. Reposting the full
carried-over list here anyway so it's captured in this thread directly, as
requested:
- **Issue 2 (= your F2, Medium, tracked, non-blocking)**: check-then-use gap
between `slotActiveCheck` and actual deploy — no Worker-side atomic claim/pin
step.
- **Issue 3 (= your F5, Low, non-blocking)**: no test drives a mixed
partial-reuse-then-release-of-non-reused-slots outcome in the cleanup branch.
**Status of every F-item**, re-verified just now directly against the
current head's real source (`e97540d9a7`, which is byte-identical to
`5730017c82` on every file this PR owns — I fetched the PR head into a local
worktree and read the files, not relying on memory of prior rounds):
- **F1 (you rated HIGH)** — Not open. This was my own mischaracterization in
the 09-02/09-06 review rounds, corrected in the 09-08 review and re-confirmed
just now by reading the method body again: `masterFailoverRestore = false;`
sits inside the single `if (enoughResource) {...}` block in
`JobMaster.preApplyResources`, which both the whole-job and subPlan paths fall
into on success — it is not gated by `isSubPlan` at all, so it fires
identically either way. There's an inline comment right above it now ("Retained
slots are valid only for the first successful allocation after failover..."),
and `testSubPlanRestoreClearsFailoverSlotReuseFlag` directly asserts this by
calling `preApplyResources(subPlan)` and checking the flag clears. So: closed,
false alarm on my part, not a live concern.
- **F2** — Still open, Medium, non-blocking (carried-over Issue 2 above).
Accepted as an out-of-scope structural follow-up (Worker-side atomic reclaim),
not something this PR needs to solve to merge.
- **F3** — Not open, confirmed false alarm against current source.
`SlotProfile.ownerJobID` is `private long` (`SlotProfile.java:37`), so the `==`
in `slotActiveCheck` is a primitive comparison, not a boxed-reference hazard.
And I grepped every call site of `slotActiveCheck` in the current head:
`JobMaster.getReusableSlot` is the only production caller; every other caller
is test code (`JobMasterMasterFailoverResourceTest`, `ResourceManagerTest`). So
there's no pre-existing production caller whose contract got silently tightened.
- **F4** — Fixed. `reusedSlotProfiles` is now `Set<SlotProfile>
reusedSlotProfiles = new HashSet<>()` (plain value equality via
`SlotProfile#equals` on worker+slotID+sequence), not the old
`IdentityHashMap`-backed set — verified directly in the current head's
`JobMaster.java`, with an inline comment explaining why value-based membership
is now safe.
- **F5** — Still open, Low, non-blocking (carried-over Issue 3 above).
- **F6** — Fixed. `getReusableSlot`'s Javadoc now carries `@param
taskGroupLocation`/`@return` and explicitly states the slot is reused only if
"the slot's owner job ID still matches this job" — verified in the current head.
- **F7** — Fixed. Both EN/ZH `resource-management.md` now read "this check
does not look at the `dynamic-slot` setting" — verified in the current head.
- **F8** — Fixed. `preApplyResourcesForAll` is now `private void` (mutates
its map argument in place, no return value to ignore) — verified in the current
head.
Net: at this head, 2 non-blocking follow-ups remain open (F2 and F5, both
previously agreed as out-of-scope-or-fast-follow, not blockers), and everything
else on your list is either fixed in a landed commit or a confirmed false alarm
(including F1, which is on me). This doesn't change my
Ready-to-merge-after-this-head's-CI-is-green conclusion from the 00:46 UTC
review — I'm not asserting CI is green as observed fact there, just that
nothing on the source side is blocking.
--
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]
