DanielLeens commented on PR #10678: URL: https://github.com/apache/seatunnel/pull/10678#issuecomment-6310045009
Thanks @SEZ9 for restating the open items on `232cb10b53e`. I owe @knight6236 a clearer picture, because my review on this head did not cover all of them, and I want to correct that (carryover on my side). I re-read `DefaultClassLoaderService` at this head against each point: - **F7 / F8 (docs: env-var form and precedence, duplicated `--add-opens` warning, JVM-global side effect):** agreed. This is the same documentation gap as Issue 3 in my review, plus the missing `classloader-cache-mode: false` requirement (my Issue 1). Please treat the docs edits as the main remaining ask. - **F2 (TCCL reset target):** agreed, and it is the same point as the Low note in my review. It is reachable only when `classloader-cache-mode` is `false` (`releaseClassLoader` returns before `recycleClassLoaderFromThread` in cache mode, `DefaultClassLoaderService.java:201-203`), so it does not touch the shipped default. A sentence in the docs and the PR description is enough for me; keeping `null` outside deep clean would also be fine. - **F1 / F6 (`JAR_CACHE_DISABLED` is set before the disable succeeds, `DefaultClassLoaderService.java:107`):** this is a fair observation that I missed. The comment above it states that a failure is deliberately never retried, so it is a documented choice, but resetting the flag in the failure paths (and passing the exception to the WARN at `:131` / `:134`, so the stack trace is kept) is cheap and removes a silent dead end. I rate it Low: it only matters on an opt-in path, and on JDK 9+ the protocol-scoped overload is a public method that is practically always present. - **F3 (JDK 8 fallback URL and "skip close if the cache disable failed"):** the `jar:file://dummy.jar!/` URL is only used to obtain a `URLConnection` instance for the static `setDefaultUseCaches(false)` call, so I consider the host-versus-path detail cosmetic. The second half is more interesting: if the disable failed, `closeUrlClassLoader` (`:314-315`) still closes the `URLClassLoader` while the JVM-wide jar cache stays on, which is the cross-reference `ZipException` scenario this PR wants to avoid. Skipping the physical close when the disable did not succeed would be a sensible guard. Low to Medium, opt-in only. - **F4 / F5 (`closeAllClassLoaders()` is gated and not refcount-aware):** confirmed from the source. `close()` calls `closeAllClassLoaders()` (`:292-298`, `:339-346`), but `closeClassLoader` does nothing unless `deepCleanEnabled` (`:314`), so shutdown-close exists only in deep-clean mode, and in that mode it closes loaders regardless of `AtomicInteger` reference counts. At node shutdown this is mostly harmless, so I see it as Low, but the docs should say that shutdown-close is deep-clean-only. **My overall read:** none of these is a blocker for an opt-in, default-off change; my merge recommendation stays "Ready to merge" on the source side, with the docs fixes (F7/F8 plus the cache-mode requirement) as the one thing I would really like to see before merge, and F1/F3 as nice-to-have hardening. @knight6236, please do not feel you must act on all eight items; the docs edits plus F1 and F3 would cover what matters most. CI is still the open gate: the apache `Build` check for this head reports that no fork run exists, and the verification run I started on my fork for this head is still queued, so there is no CI verdict yet. -- 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]
