JeremyXin commented on PR #11458:
URL: https://github.com/apache/seatunnel/pull/11458#issuecomment-5433426303
Reviewed the latest head `bbaf984dc`.
The current version fixes the two main correctness risks I was looking for
during master failover recovery:
1. reused fixed slots are only accepted when the worker still reports the
same slot identity and the same owner job;
2. partial pre-allocation failure no longer releases retained slots that
should survive for the next retry.
### Merge Conclusion: Approve to merge
A few minor, non-blocking suggestions below — feel free to address in a
follow-up if you prefer:
### Minor Suggestions
#### 1. Add a comment explaining the `IdentityHashMap` choice
**Location:** `JobMaster.java` — `preApplyResources()`, `reusedSlotProfiles`
declaration
Since `SlotProfile.equals()` compares `worker`+`slotID`+`sequence` but
excludes `ownerJobID`, identity comparison is needed to distinguish reused
slots from newly allocated ones. A brief inline comment would prevent someone
from accidentally replacing it with a `HashSet` later.
#### 2. Add a JobMaster-level log when a slot is reused
**Location:** `JobMaster.java` — `getReusableSlot()`
Currently only `AbstractResourceManager.slotActiveCheck()` logs at the
ResourceManager level. An INFO log in `getReusableSlot()` when returning a
non-null slot (with jobId, taskGroupLocation, slotProfile) would improve
operational visibility during failover recovery.
#### 3. Extract shared coordinator/task vertex logic
**Location:** `JobMaster.java` — `preApplyResourcesForSubPlan()`
The coordinator and task vertex loops contain near-identical reuse/apply
logic (~15 lines each). Could be extracted into a small helper like
`applyOrReuseForVertex(...)` to reduce duplication.
#### 4. Clarify `masterFailoverRestore` reset scope
**Location:** `JobMaster.java` — `preApplyResources()`, the `!isSubPlan`
success branch
The flag is only reset on the full-plan success path, not the SubPlan path.
I believe this is safe since SubPlan-level applies follow after the full-plan
apply has already reset it, but a one-line comment noting this is intentional
would help future readers.
---
Overall, LGTM 👍
--
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]
