DanielLeens commented on PR #11757:
URL: https://github.com/apache/seatunnel/pull/11757#issuecomment-5379398738
Thanks @SEZ9 — appreciate you keeping this moving, but I want to correct one
detail before it causes confusion for @waterWang: the blocker you quoted
("class-loader release under the shared deployment lock") was the blocker as of
my 10:47Z review that day, but it's not the current one. My very next review
that same day (13:56Z, on the byte-identical head `49f77109f`) downgraded that
lock-scope concern to a non-blocking recommended fix, because on closer
inspection it's gated behind the non-default `classloader-cache-mode=false` and
doesn't reopen the race — it's a latency-amplification risk, not a correctness
break.
In that same 13:56Z pass I found a different, new issue that is the actual
current blocker: on the rejected/stale branch of `finishOwnedResources()`
(`TaskExecutionService.java:1454-1462`), when `finishExecutionContext()`
returns `false` (i.e. exactly the stale-generation case this PR is built to
detect), the method logs a warning and returns *without* ever calling
`recycleClassLoader(taskGroupLocation, ownedContext)` for its own context.
`recycleClassLoader()` already takes `ownedContext` as an explicit argument
rather than doing a map lookup, so it's always safe to call on the rejected
path too — it only ever releases the stale tracker's own class loader, not
whatever now occupies the map slot. Under the default
`classloader-cache-mode=true` this is a no-op either way, but under
`classloader-cache-mode=false` (per-job isolation) it's a genuine unbounded
reference-count leak: every time the race this PR targets actually fires, one
class loader's reference count never reaches ze
ro and is never evicted.
So, to restate precisely for the record: the one remaining blocker is adding
a `recycleClassLoader(taskGroupLocation, ownedContext)` call to the rejected
branch before the `return` at `TaskExecutionService.java:1462`. The lock-scope
item and the #11727 coordination item are both non-blocking recommended fixes,
not blockers.
Agreed on the plan otherwise: once @waterWang rebases onto current `dev`,
I'll do a full fresh pass on the refreshed head rather than just diffing the
delta, same as every round so far. Thanks for pushing this along for the 3.0.0
release effort.
--
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]
