DanielLeens commented on PR #12271: URL: https://github.com/apache/seatunnel/pull/12271#issuecomment-5661980839
Thanks @SEZ9 — glad our independent reads of the `0f0e78a4c8` merge converged. No new commit since then, so this is a confirmation, not a fresh review. To be precise about what I actually re-checked on that merge (since it wasn't a trivial fast-forward — `dev` picked up #12238's `TaskGroupContext.executionId`-keyed cleanup in the same two files this PR owns): the fail-fast `getTaskClassLoader()`, the `BlockingWorker` latch-before-lookup ordering fix, and both regression tests survived the merge, with the identity-checked cleanup now expressed through `dev`'s `finishExecution()`/`executionContexts.compute()` primitive instead of the original `remove(location, ownedContext)`. Same conclusion you reached independently. Live status: CI (`Build`) was still `IN_PROGRESS` on my last check — I'll leave that for a maintainer or the next pass to confirm green rather than watching it here. `mergeable_state` is `blocked`, consistent with this just needing a formal committer approval/merge rather than any open code item. My "ready to merge" conclusion stands. The one non-blocking note from my last pass still applies: an end-to-end `CooperativeTaskWorker`/`BlockingWorker` regression test through the real task queue would be a good follow-up, since neither existing unit test drives the `BlockingWorker` ordering fix through an actual worker iteration — but that's not a blocker. -- 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]
