DanielLeens commented on PR #11727: URL: https://github.com/apache/seatunnel/pull/11727#issuecomment-5674346620
Thanks for the two pointers, @SEZ9 — pulled the current head (`01a7052409e5`) directly to answer both precisely rather than from memory. **1. Redeploy-vs-taskDone race on `cancellationFutures`.** `TaskExecutionServiceTest#testStaleTaskDoneCleansOnlyOwnedGenerationResources` (lines 573-655) is the one that exercises this. It builds an old-generation `TaskGroupExecutionTracker` and a new-generation one sharing the same `TaskGroupLocation`, seeds both `cancellationFutures` entries directly, then calls `oldTracker.taskDone(oldTask)` and asserts: - `cancellationFutures.containsKey(oldContext)` is now `false` (old entry removed) - `cancellationFutures.get(newContext)` is still the original `newCancellationFuture`, untouched, and it is not cancelled - the same pattern for `asyncFutures`/`timerFlushFutures` (old cancelled, new untouched) and `executionContexts` (still points at `newContext`) So it directly pins the exact scenario you're asking about — a stale generation's `taskDone()` firing after a newer generation has already redeployed under the same location — across all four maps, not just `executionContexts`. **2. F1/F3 wording — the monitor claim isn't there to check anymore.** The design changed since the round where that Javadoc language existed. On the current head there is no `synchronized (TaskExecutionService.this)` block and no "same monitor as deployment" claim at all: - `finishExecution()` (`TaskExecutionService.java:1685-1729`) does the active-context handoff via `executionContexts.compute(taskGroupLocation, (ignored, activeContext) -> ...)`, comparing `context.equals(activeContext)` inside the compute lambda — `ConcurrentMap.compute` is what serializes the transition, not a monitor. The Javadoc above it now just says "Only the active context mapping is conditional. All other resources are keyed by this tracker's context and can therefore always be cleaned safely," which matches what the code actually does. - `deployLocalTask()`'s publication (lines 705-724) is explicit about this in its own comment: "No service-wide lock is required: publication is one-way, and cleanup uses the context's execution ID rather than TaskGroupLocation to address the exact deployment." The two-phase publish (register every resource under the new context first, then `executionContexts.put(...)` last) relies on `ConcurrentHashMap` happens-before visibility, not a shared lock. So there's no stale monitor-vs-install mismatch to find on this head — the approach moved from lock-based to identity/compute-based between the round that Javadoc text came from and now. If you spot different wording anywhere else in the diff, a pointer would help, but I couldn't find any remaining monitor/synchronized claim tied to context installation on `01a7052409e5`. Nothing changes about my last full review's "ready to merge" conclusion (pending your pass) — just closing out these two so you're not hunting blind. -- 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]
