SEZ9 commented on PR #10678: URL: https://github.com/apache/seatunnel/pull/10678#issuecomment-6307323067
Thanks for sticking with this through the long cycle. I looked at head `232cb10b53e` again against the points raised on `f111e48da8a`, and they still apply as written. Remaining asks: 1. **PR10678-F1 / F6 (`JAR_CACHE_DISABLED` CAS):** set the flag only after the disable actually succeeds (or reset it on failure) so a failed attempt can be retried; if the JDK 9+ reflective disable throws, fall through to the JDK 8 global toggle instead of giving up; and attach the exception to the WARN so the stack trace is kept. 2. **PR10678-F2 (TCCL `null` -> `getSystemClassLoader()`):** this is an unconditional semantic change on the default path in non-cache mode. Please either keep `null` on the default path and use the system loader only under deep clean, or call the change out explicitly in the docs and PR description, and consider the original parent/app loader rather than the system loader for non-standard launch setups. 3. **PR10678-F3 (JDK 8 fallback):** `jar:file://dummy.jar!/` puts `dummy.jar` in the host component; please use a form where it is in the path. If the fallback fails, deep clean should skip `URLClassLoader.close()` rather than close JarFiles while the JVM-global cache is still enabled — that is exactly the cross-reference `ZipException` this PR is meant to remove. 4. **PR10678-F4 / F5 (`closeAllClassLoaders()`):** on the default path it is a no-op because `closeClassLoader()` is fully gated by `deepCleanEnabled`, so "close on service shutdown" never happens unless deep clean is on; in deep-clean mode it closes loaders without checking outstanding refcounts. Please make shutdown close refcount-aware (or at least log/skip loaders with refCount > 0), and align the docs so the shutdown-close behavior is stated as deep-clean-only. 5. **PR10678-F7 / F8 (docs):** document the `SEATUNNEL_CLASSLOADER_DEEP_CLEAN` environment-variable form and its precedence relative to the system property in both the EN and ZH doc; dedupe the repeated "do not add `--add-opens` on JDK 8" warning in the EN doc; and add a sentence noting that the JDK 9+ jar-protocol cache disable is a JVM-global side effect. Happy to take another pass once these are addressed. <!-- streview-comment:1918 --> -- 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]
