DanielLeens commented on PR #10678:
URL: https://github.com/apache/seatunnel/pull/10678#issuecomment-5229130462
@knight6236 thanks for the detailed point-by-point response — this addresses
both blockers from my last review cleanly.
On the JDK 9+ gap: updating the WARN log and docs at
`DefaultClassLoaderService.java:324` to include `--add-opens
java.base/jdk.internal.loader=ALL-UNNAMED` alongside the existing
`java.base/java.net` flag closes the loop I raised — that was the missing piece
that made the documented flag insufficient on JDK 9+.
On the JDK 8 `useCaches` issue: agreed this is a genuine JDK 8 API
limitation rather than a bug in your logic, and the Tomcat
`JreMemoryLeakPreventionListener` precedent is a fair comparison — they hit the
same protocol-scoping gap on Java 8 and made the same call. Adding the JDK 9+
`URLConnection.setDefaultUseCaches("jar", false)` path plus explicit
documentation/startup-log warnings for the JDK 8 case is the right shape of
fix: it doesn't try to make JDK 8 do something the platform can't do, it just
makes the tradeoff visible to operators instead of silent.
On keeping `clearJarFileFactoryCache()` out of scope: that also matches what
I'd want to see — your ConcurrentModificationException experience with
reflectively mutating the JVM-global `JarFileFactory` cache under concurrent
class loading is a real hazard, and the dual-switch design (URL-cache-disable
vs. deep-clean, both default-off) keeps that risk fully opt-in without
reopening the cross-job hazard I flagged back in April.
One procedural note: since these fixes are described as "next steps" and
haven't landed as a commit yet, I don't have a new head to verify against — the
classloader files are still unchanged from `89ff5c3f63c2`, which is the
revision my last CHANGES_REQUESTED review applies to. I'll hold that review
as-is until the commit lands, then do a focused re-check of the JDK 9+ flag
text and the JDK 8/9+ cache-scoping change together (ideally with the
regression test you mentioned for the protocol boundary, e.g. asserting an HTTP
`URLConnection`'s default caching is unchanged after service construction on
JDK 8).
@SEZ9 also +1 on your confirmation that the `disableJarUrlCache()` catch
block doesn't override the deep-clean flag — matches what I verified
independently. Your two remaining asks (the user-facing docs for
`SEATUNNEL_CLASSLOADER_DEEP_CLEAN`, and a re-look at the dynamic-JAR test
stability once pushed) line up well with what I'd want covered in the same
pass, so we can review the new commit together once it's up.
@knight6236 happy to re-review as soon as the commit is pushed — no need to
guess priority between the two blocking items, both can land together as you
planned.
--
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]
