davidzollo commented on PR #11458: URL: https://github.com/apache/seatunnel/pull/11458#issuecomment-5509598218
Re-reviewed the outstanding open points from SEZ9's 2026-08-31 review and nzw921rx's 2026-08-27 review against the current head (68640a2e4). Already resolved by earlier commits on this branch, verified against current source: - SEZ9 Issue 1 (High/Blocking) - `masterFailoverRestore` not cleared on the subPlan path: fixed by 43cd0c44e; the flag is now cleared inside the shared `enoughResource` success branch that both the whole-job and per-pipeline paths funnel through, so it is unconditional on `isSubPlan`. - nzw921rx's [P2][Blocking] same-job slot reassignment: fixed by f7bf00cbc; `DefaultSlotService#requestSlot` now assigns a fresh random UUID as the allocation sequence on every successful assignment instead of reusing the constant worker-level sequence, so a slot reassigned to another task group of the same job no longer satisfies a stale persisted mapping. A dedicated regression test (`testRestoreRejectsSlotReassignedWithinSameJob`) covers this in `JobMasterMasterFailoverResourceTest`. Verified as non-issues (no code change needed): - SEZ9 Issue 3, second half: `ownerJobID` is a primitive `long` on both sides of the `==` comparison in `AbstractResourceManager#slotActiveCheck` (not boxed `Long`), so the comparison is numeric equality, not reference equality - safe as written. - SEZ9 Issue 3, first half: `slotActiveCheck` currently has exactly one production caller (`JobMaster#getReusableSlot`), so there is no other existing caller whose contract the added owner-job-ID check could silently tighten. Fixed in 04a6b4612 (pushed just now): - SEZ9 Issue 4 (Medium): switched the `reusedSlotProfiles` guard set in `JobMaster#preApplyResources` from an `IdentityHashMap`-backed set to value-based membership, since `SlotProfile#equals` already keys on worker+slotID+sequence and every allocation now gets a fresh sequence token (per the f7bf00cbc fix above). This removes the dependency on the exact same object instance flowing from pre-apply through cleanup. - SEZ9 Issue 8 (Low): `preApplyResourcesForAll` is now `void`; it only mutates the map passed in and its return value was already discarded at the only call site. - SEZ9 Issue 6 (Low): completed the `getReusableSlot` Javadoc with `@param`/`@return` tags and documented the owner-job-ID condition enforced by `slotActiveCheck`. - SEZ9 Issue 7 (Low): corrected the EN/ZH resource-management docs - slot reuse after an active-master failover does not check `dynamic-slot`; fixed slots are simply the case that benefits most because re-requesting one the Worker still holds would otherwise fail. Left open for follow-up (not fixed in this pass): - SEZ9 Issue 2 (Medium): making slot reuse a worker-side atomic reclaim (validate+re-pin on the Worker's SlotService) instead of a read-only heartbeat-registry check is a protocol-level change beyond the scope of a review-comment fix pass; recommend tracking separately. - SEZ9 Issue 5 (Low): no test currently drives the partial-reuse failure-cleanup path (mix of reused and freshly-allocated slots, or an ownership-changed slot, with insufficient free capacity). Left for the PR author/a follow-up, since authoring a new failover scenario test without the ability to run it locally in this pass risked introducing an unverified/flaky case into this financial-scale code path. No inline review threads exist on this PR (both reviews are top-level review-body comments with no line-level comments), so there is nothing to resolve via the thread API. CI: a fresh Build run was triggered by the push above; previous state was `pending` before this push. -- 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]
