DanielLeens commented on PR #11727:
URL: https://github.com/apache/seatunnel/pull/11727#issuecomment-5476992296
Thanks for taking the time to re-check this against #11757's F1-F8, @SEZ9 —
that's exactly the follow-up @abdessalems asked for on the 30th. I went and
re-read the actual current head (`d98cfb60854b`, same SHA you cited) end to end
before replying, rather than relying on my earlier pass, since your conclusion
("verification is incomplete; approval is not ready") is a real disagreement
with where I'd landed. Here's what I found line-by-line.
**Issues 1, 2 and 3 (your blocking items) don't reproduce against the
current head — the code already does what you say is missing.**
- Issue 1/3 claim `deployLocalTask()` installs
`executionContexts`/`cancellationFutures` without taking
`TaskExecutionService.this`. Checking the actual deploy path:
```java
// TaskExecutionService.java:674-678
synchronized (this) {
executionContexts.put(taskGroup.getTaskGroupLocation(), context);
cancellationFutures.put(taskGroup.getTaskGroupLocation(),
cancellationFuture);
contextPublished = true;
}
```
This is inside `TaskExecutionService`, so `synchronized (this)` **is**
`synchronized (TaskExecutionService.this)` — the same monitor
`finishOwnedResources()` takes at line 1559. The Javadoc on
`finishOwnedResources()` ("under the same monitor used by deployment", now at
lines 1548-1553, not 1494 where you point) matches the code as written.
- Issue 2 claims `BlockingWorker.run()` resolves its context via
`executionContexts.get(taskGroupLocation)` (your line 1086/1094). The actual
current code:
```java
// TaskExecutionService.java:1136-1138
// Each tracker retains its own generation's context, so a queued worker
never
// resolves a class loader from a replacement generation sharing this
location.
TaskGroupContext taskGroupContext = taskGroupExecutionTracker.ownedContext;
```
It reads `tracker.ownedContext`, not the shared map — this is precisely
the fix your own #11757 Issue 2 asks for, and it's already here, with a comment
explicitly calling out why.
- Issue 6 (heavy teardown under the lock) doesn't hold either:
`recycleClassLoader(...)` (line 1570) and
`cancelAsyncFunctions`/`cancelTimerFlushTasks` (1579, 1584) all run **after**
the `synchronized` block closes at line 1569 — only the ownership check and the
three map removals are inside the monitor.
- Issue 8's "redundant get before remove" doesn't match current code either
— `finishExecutionContext()` (1594-1600) goes straight to
`executionContexts.remove(taskGroupLocation, ownedContext)` with no preceding
`.get()`.
I want to flag something directly rather than let it sit implicit: every
line number in your review (592, 1086, 1094, 1494, 1503, 1513-1519, 1524,
1536-1541) is off by 80-150 lines from where the equivalent logic actually sits
in the current 1668-line file (674-678, 1136-1138, 1548-1614), and the code
shape itself has moved past what those issues describe — this reads like the
F1-F8 set was carried over from #11757's findings rather than freshly re-traced
against `d98cfb60854b` as the opening line says. Given the two items marked as
blocking (Issues 1 and 2) are the ones that don't reproduce, I'm not walking
back my "ready to merge" conclusion on this basis — but I'd genuinely like you
to re-check directly against the current head before we ask @abdessalems for
another round, since the fix these describe is already in.
**Where I think you may still have a point — Issue 4.**
This is the one item whose structural description still matches current
code, just at new line numbers: in the stale branch of `finishOwnedResources()`
(1571-1577), `recycleClassLoader()` runs unconditionally, then the method logs
and returns *before* reaching `cancelAsyncFunctions`/`cancelTimerFlushTasks`. I
traced where the entries actually go, though:
`taskAsyncFunctionFuture`/`timerFlushFutures` are keyed by `taskGroupLocation`
with a shared inner map per location
(`taskAsyncFunctionFuture.get(taskGroupLocation).put(id, future)`, line 793) —
so if the new generation has already registered async functions under the same
location by the time the old tracker's stale branch runs, they land in the
*same* inner map object, and the new generation's own `finishOwnedResources()`
will eventually remove-and-cancel that whole map, old entries included. So I
don't think this is an unbounded leak the way Issue 4 phrases it — worst case
it's a bounded delay (old generation's async functi
ons/timer flushes get cancelled when the new generation finishes, not
immediately). Still worth a closer look together with Issue 5, but I wouldn't
call either one blocking on its own.
**Issues 5 and 7** — Issue 5's "zombie tasks holding connector resources"
concern doesn't materialize in practice either, since `recycleClassLoader()`
always runs against the tracker's own `ownedContext` field regardless of what's
currently in the shared map, so class-loader release isn't affected by the
overwrite. Issue 7 (test coverage / reflection) is a fair test-quality note
independent of whether the underlying race exists — happy to see a follow-up
test for the `cancellationFutures` interleaving either way.
Given the above, my read stands: the two blocking items don't apply to
`d98cfb60854b`, so I don't think this needs to go back to "not ready" on their
account. @abdessalems — nothing needed from you on this one until @SEZ9 has a
chance to confirm against the actual current line numbers; I don't want you
chasing a fix for code that's already there.
--
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]
