SEZ9 commented on PR #11757: URL: https://github.com/apache/seatunnel/pull/11757#issuecomment-5366243112
Thanks @DanielLeens for the detailed re-reviews and for confirming the direction of the current fix — binding `ownedContext` as a `final` field at construction and running the full teardown in `finishOwnedResources()` under the same `synchronized (TaskExecutionService.this)` monitor that `deployTask()` holds is indeed a materially stronger shape than the earlier map-entry-only guard. To keep this moving, here is where I see things and the concrete remaining asks: 1. **Rebase/refresh the branch.** As noted in the latest comment, the temporary follow-up PR has been closed and we want to continue on this PR. Please rebase or otherwise update this branch onto the latest `dev` and push a refreshed head so review and CI follow-up can continue here directly. 2. **Outstanding Medium blocker.** Since `49f77109f` is an empty diff on top of `a8407319b` (per the re-review), the Medium blocker from the 10:47Z review — the class-loader release under the shared deployment lock — is still open as far as this thread shows. Please either address it in the refreshed head or explain why the current handling is safe, and we can settle it in the next pass. 3. **The two non-blocking Medium/Low items** from that same review can be handled in this PR or deferred — just note your preference when you push. Once the rebased head is up, we'll do the next review round and CI follow-up on this PR. Appreciate the work here — this is close, and it's on the path for the 3.0.0 release effort. <!-- streview-comment:421 --> -- 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]
