SEZ9 commented on PR #11727: URL: https://github.com/apache/seatunnel/pull/11727#issuecomment-6379854016
Thanks for the CI digging on head `a80834ebfb185ee0d54243e7a3ae99a5bf481817` (run 35426701856). The engine-v2-it failures (`BackpressureSlowSinkIT.testCheckpointsKeepCompletingUnderSustainedBackpressure` on jobs 105854144544 / 105854144606, and `CheckpointCoordinatorFailoverIT.testStreamJobFailsAfterCheckpointTriggerDispatchFailure` on 105854144606) exercise checkpoint completion/failure paths close to the task-lifecycle code this PR touches, so I'd rather not treat them as infra noise without a baseline run of the same scenarios on the base branch. The CDC failures on 105854144530 / 105854144535 / 105854144377 look further from this change, but the same comparison would settle that too. No causality claim either way yet — please attach the comparison with the next push. On the review items, these still look open to me: - **F1 / F3 / F5 (deploy vs. taskDone race):** `deployLocalTask()` still installs the context and cancellation future without taking the `TaskExecutionService.this` monitor that `finishOwnedResources()` says it shares, and does so with a plain put. Either take the monitor around the install and use a conditional insert so a still-live older generation can't be silently overwritten, or rewrite the Javadoc to describe what is actually guaranteed. The race on cancellation futures / async functions / timer flushes isn't closed until the install side participates in the same lock. - **F2 (BlockingWorker context resolution):** `BlockingWorker` should resolve its context from the tracker's `ownedContext`, not from the shared `executionContexts` map; otherwise a reused `TaskGroupLocation` can hand it a newer generation's context/class loader. Could you confirm what that path does on this head? - **F4 / F8 (stale-cleanup branch):** the stale branch recycles class loaders and returns, but never cancels the old generation's async functions or timer-flush tasks, and never moves `ownedContext` into `finishedExecutionContexts`. Please add both. While there, `finishExecutionContext()` can drop the redundant get before the conditional remove. - **F6 (teardown under the service monitor):** class-loader release and async/timer cancellation now run while holding the service-wide monitor. Only the map mutations need the lock; please hoist the heavy work outside it once F1/F5 settle the ordering. - **F7 (test coverage):** `TaskDeployStaleContextRaceTest` covers the deploy-returns contract but not the redeploy-vs-taskDone race on the cancellation futures. A test for that case would be good, ideally via package-private hooks rather than reflection into `TaskExecutionService` internals. Once those land along with the CI baseline comparison, I'll take another full pass. <!-- streview-comment:1999 --> -- 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]
