SEZ9 commented on PR #10678: URL: https://github.com/apache/seatunnel/pull/10678#issuecomment-6332042960
Following up on the open items against `232cb10b53e`, here is where I stand on each and what I'd like to see before the next pass: **F7 / F8 (docs)** — Still the main remaining ask. Please add to both the EN and ZH docs: the `SEATUNNEL_CLASSLOADER_DEEP_CLEAN` environment-variable form and its precedence over the `-D` system property, the `classloader-cache-mode: false` requirement, a note that on JDK 9+ deep clean disables the JVM-global `jar:` protocol URL cache as a side effect, and collapse the duplicated "do not add `--add-opens` on JDK 8" warning into a single section. **F2 (TCCL reset target)** — Agreed this is only reachable when `classloader-cache-mode` is `false` (`releaseClassLoader` returns before `recycleClassLoaderFromThread` in cache mode, `DefaultClassLoaderService.java:201-203`), so it does not affect the shipped default. Either keep `null` outside deep clean, or keep `getSystemClassLoader()` and document the change in the doc and the PR description — your call, just please make it explicit. **F1 / F6 (`JAR_CACHE_DISABLED` CAS at `:107`)** — The comment documents the no-retry choice, so I'll treat this as Low, but please reset the flag in the failure paths and pass the exception to the WARN at `:131` / `:134` so the stack trace is preserved. Cheap change, removes a silent dead end. **F3 (JDK 8 fallback)** — The dummy URL's host-vs-path detail is cosmetic since it only exists to obtain a `URLConnection` for the static `setDefaultUseCaches(false)` call; I'm fine leaving it. The important half: if the disable failed, `closeUrlClassLoader` (`:314-315`) still physically closes the `URLClassLoader` while the JVM-wide jar cache is active, which is the exact `ZipException` hazard this PR targets. Please skip the physical close when the disable did not succeed (and log why). **F4 / F5 (`closeAllClassLoaders()`)** — Confirmed: `close()` calls `closeAllClassLoaders()` (`:292-298`, `:339-346`) but `closeClassLoader` is a no-op unless `deepCleanEnabled` (`:314`), so shutdown-close only exists in deep-clean mode, and in that mode it ignores outstanding reference counts. Please either (a) make the PR description / docs state that shutdown-close is deep-clean-only, or (b) make it unconditional — and in either case have `closeAllClassLoaders()` respect non-zero reference counts (skip or log-and-defer) so in-flight task teardown isn't broken. Once the docs edits and the F1/F3/F5 guards are in, I'm happy to re-review. <!-- streview-comment:1950 --> -- 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]
