SEZ9 commented on PR #11458:
URL: https://github.com/apache/seatunnel/pull/11458#issuecomment-5595222571
Thanks @DanielLeens for the re-verification pass.
Based on your description, `e97540d9a7` is a merge of the current `dev` tip
`438f9ccfbef` into `5730017c82`, and the diff restricted to this PR's changed
files is empty, so there is nothing new to review on the merits at this head. I
am fine treating it as a no-op checkpoint, with your `5730017c82` review as the
reference point.
Two requests:
1. Your comment appears to be cut off mid-sentence in section 1.4
("Carried-over, non-blocking items from my la…"). Could you repost the rest so
the carried-over list is captured here rather than inferred?
2. Could you give a one-line status (still open / addressed / withdrawn) for
each of the previously raised findings? Since this head does not touch the PR's
own code, I assume they are unchanged from `5730017c82`, but please confirm or
correct:
- F1 (HIGH): `masterFailoverRestore` only cleared on the whole-job
pre-apply success branch, not on the subPlan/pipeline path. You mention
correcting an inaccuracy in your earlier notes about where the flag is cleared
— does that change your view on whether F1 is still open?
- F2 (MEDIUM): check-then-use of heartbeat-eventually-consistent registry
state with no worker-side reclaim.
- F3 (MEDIUM): `slotActiveCheck` ownerJobID tightening changing the
contract for existing callers and relying on `==`.
- F4 (MEDIUM): identity-based `reusedSlotProfiles` set fragility on the
failure/cleanup path.
- F5 (LOW): missing partial-reuse failure cleanup and ownership-changed
fallback test coverage.
- F6 (LOW): `getReusableSlot` Javadoc missing parameter/return tags and
the owner-job-ID condition.
- F7 (LOW): docs describe reuse as fixed-slot-specific while
`getReusableSlot` applies regardless of slot mode.
- F8 (LOW): `preApplyResourcesForAll` returning its mutated input map
with the return value ignored at the call site.
F1 in particular I'd like to see either resolved or explicitly argued down
before this moves forward; the rest can stay as non-blocking follow-ups if that
matches your carried-over list.
<!-- streview-comment:920 -->
--
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]
