davidzollo commented on PR #11757:
URL: https://github.com/apache/seatunnel/pull/11757#issuecomment-5509606828
## CI diagnosis and a follow-up fix from the outstanding review feedback
**CI failure:** the `Build` check on head `863ddb17` was failing in `Run /
all-connectors-it-6 (11, ubuntu-latest)`. The actual error is
`DatabendCDCSinkIT` throwing `ContainerLaunchException` while starting the
`datafuselabs/databend:nightly` testcontainer:
```
Databend Query start failure, cause: StorageUnavailable ... config:
S3(StorageS3Config { endpoint_url: "http://minio:9000";, ... })
Caused by: java.net.SocketException: Connection reset
```
This is a Testcontainers/Docker-networking flake between the Databend query
container and its MinIO-backed storage container, in `connector-databend-e2e` —
a module this PR does not touch (this PR's only files are
`TaskExecutionService.java` and `TaskExecutionServiceTest.java`). @DanielLeens
independently reached the same conclusion in his 2026-08-29 review ("This reads
as unrelated infra flakiness, not a regression from this change — worth a rerun
rather than a fix here"). I don't have write access to the fork to rerun the
specific job directly (`gh run rerun ... --job` returned "Must have admin
rights to Repository"), so the push below also serves to get a fresh CI run.
**Follow-up fix pushed (`710c6e2`):** re-reading @DanielLeens's
2026-08-29T11:45:53Z review against the current head, one High-severity point
he raised is still present in `TaskExecutionService.java` as of `863ddb17`:
`finishExecutionContext()`'s ownership check relied solely on
`ConcurrentMap#remove(key, value)`, which falls back to
`TaskGroupContext.equals()` when the map entry isn't reference-equal to
`ownedContext`. Since `TaskGroupContext` is a Lombok `@Data` class, that
`equals()` is structural (over `taskGroup`/`classLoaders`/`jars`), not
identity. It's safe *today* only because `TaskGroupDefaultImpl` doesn't
override `equals()`/`hashCode()` — a future `TaskGroup` implementation that
gains a structural `equals()` would silently turn this into a value comparison
and reintroduce the exact stale-generation-corrupts-newer-generation race this
PR fixes, with no compiler error and no test to catch it. I verified this
directly against the source (`TaskGroupContext.java:27-33`, `
TaskGroupDefaultImpl.java`) rather than taking the review's word for it.
I restored the explicit reference-identity check before the value-based
`remove`, matching what sibling PR #11999 already has and what the method's own
Javadoc already claims:
```java
private boolean finishExecutionContext(TaskGroupLocation taskGroupLocation) {
if (executionContexts.get(taskGroupLocation) != ownedContext) {
return false;
}
if (executionContexts.remove(taskGroupLocation, ownedContext)) {
finishedExecutionContexts.put(taskGroupLocation, ownedContext);
return true;
}
return false;
}
```
This is safe and adds no new race: the whole method already runs inside the
`synchronized (TaskExecutionService.this)` block held by
`finishOwnedResources()`, the same monitor `deployLocalTask()` takes when
publishing `executionContexts`, so the `get()`-then-`remove()` pair stays
atomic relative to a concurrent deploy of a newer generation. No behavior
change on the current safe path (today `cv == ev` always holds, so this is a
no-op there) — it just makes the invariant explicit instead of resting on an
unstated property of an unrelated class.
**Other open review items** (from @DanielLeens's and @SEZ9's threads) are
Medium/Low, explicitly called out by both reviewers as non-blocking follow-ups,
and left as-is in this push to keep the diff minimal:
- Narrowing the `finishOwnedResources()` critical section so
`recycleClassLoader()`/`cancel()` calls run outside the lock.
- Generation-scoping `taskAsyncFunctionFuture`/`timerFlushFutures` ownership
the same way `executionContexts` now is.
- `try/finally` cleanup of the synthetic map entries the new regression test
injects into the shared `server` instance.
- The `contextPublished`-guarded failure-unwind path in `deployLocalTask()`
still removing by key alone.
- Wrapping `recycleClassLoader()` in try/catch inside
`finishOwnedResources()`.
- Unit coverage for the
`acquiredClassLoaderJars`/`releaseClassLoadersAfterFailedDeployment`
failed-deployment cleanup added in `863ddb17`.
- The #11727 / #11999 coordination question (both still open, per the review
threads).
None of these were raised as blockers by either reviewer, and each is a
larger, separate design decision (lock scoping, per-future ownership tracking,
etc.) rather than a small, independently verifiable nit, so I've left them for
a deliberate follow-up rather than guessing at scope here.
**Status:** pushed `710c6e2` on top of `863ddb17`. New CI run queued on the
fork. This should now be blocked only on CI passing and a maintainer merge
decision (including the #11727/#11999 sequencing question raised in the review
threads).
--
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]
