SEZ9 commented on PR #11458: URL: https://github.com/apache/seatunnel/pull/11458#issuecomment-5564294625
Thanks @DanielLeens for the detailed disclosure on the `02839c50d9b6` head. I agree with your reading: `4c539e330` and `02839c50d` are empty retrigger commits and `git diff 65009e5c60..02839c50d9b6 --stat` is empty, so there is nothing new to re-derive on the source side. That said, an unchanged diff also means the previously raised points are still open. Before this can move forward I need an explicit response (fix or rationale) on each: - **PR11458-F1 (HIGH, Logic)** — `masterFailoverRestore` is only cleared on the whole-job pre-apply success branch in `JobMaster.java`; the subPlan/pipeline path never clears it, so slot reuse can fire on ordinary pipeline restarts long after failover. This is the blocker; please clear the flag on the pipeline path too, or explain why that path is unreachable while the flag is set. - **PR11458-F2 (MEDIUM, Robustness)** — slot reuse is a check-then-use of heartbeat-eventually-consistent registry state with no worker-side reclaim. How do you see the window right after failover (slots genuinely retained but not yet reported), and the dead or mid-reassignment worker case? - **PR11458-F3 (MEDIUM, Compatibility)** — the added `ownerJobID` equality in `slotActiveCheck` (`AbstractResourceManager.java`) changes the contract for existing callers and relies on `==`. Please use a proper equality comparison and confirm existing callers are unaffected. - **PR11458-F4 (MEDIUM, Bug)** — the identity-based `reusedSlotProfiles` set is fragile: if the reused `SlotProfile` reference is replaced between reuse and cleanup, the failure path releases a worker-retained slot and reintroduces #11437. Keying on a stable identifier would remove this. - **PR11458-F5 (LOW, Test)** — `JobMasterMasterFailoverResourceTest.java` covers only the happy full-reuse path. Please add the partial-reuse failure cleanup case (reused slots survive a failed pre-apply) and the ownership-changed fallback at JobMaster level. - **PR11458-F6 / F7 (LOW, Docs)** — the `getReusableSlot` Javadoc is missing param and return tags and the owner-job-ID condition; `docs/en/architecture/engine/resource-management.md` should not describe reuse as specific to `dynamic-slot: false` when `getReusableSlot` applies to any retained slot. - **PR11458-F8 (LOW, Style)** — `preApplyResourcesForAll` returns its mutated input map and the return value is ignored at the only call site; either use it or drop the return value. A single follow-up commit covering F1, F3 and F4 plus the F5 tests, with the doc/style items folded in, would be ideal. If you disagree with any of these, a short note per finding is fine — I just need the status stated explicitly rather than inferred from an unchanged diff. <!-- streview-comment:855 --> -- 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]
