Rangsh commented on PR #12218: URL: https://github.com/apache/seatunnel/pull/12218#issuecomment-5612321383
@DanielLeens Thanks for the re-review on `258a779d7` — especially for confirming Issue 1 is resolved and for the precise Issue 2 race analysis on the thread-share path. I pushed a follow-up (`16111de97`) that closes the recommended Issue 2 fix: - In `CooperativeTaskWorker.run()`, immediately after dequeue and **before** `currRunningTaskFuture.put(...)` / classloader lookup, add the same early bailout as `BlockingWorker`: if `executionContexts.get(loc) == null` or `isCancel`, call `taskDone` and `continue`/`break` - Keep the existing `executionCompletedExceptionally()` short-circuit in that same guard - Use the already-resolved `TaskGroupContext` for the classloader lookup instead of re-fetching from the map On Issue 3 (rolled-back attempts skipping `finishedExecutionContexts` / `updateMetricsContextInImap()`): agreed this is diagnostics-completeness only; accepting it as-is for this PR rather than broadening scope. On the flagged `connector-file-local-it (11)` / `LocalFileIT.testLocalFileBinaryUpdateModeContinuousDiscoveryWithNonRecursiveScan` failure: this push will re-trigger CI. I also traced no mechanism by which the new `BlockingWorker`/`CooperativeTaskWorker` early-bailout would delay a normal (non-rollback) cancel of a long-running continuous job, but I'll watch that specific job on the new run and investigate if it recurs. Local: `TaskExecutionServiceTest` — `Tests run: 17, Failures: 0, Errors: 0, Skipped: 0`. Please take another look when you have time. Thanks again! -- 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]
