SEZ9 commented on PR #10678: URL: https://github.com/apache/seatunnel/pull/10678#issuecomment-5225133786
Thanks @knight6236 for the quick turnaround on the test improvements — dynamically compiling a test JAR at runtime to avoid parent-delegation masking is exactly the right approach, and asserting both `loadClass()` throwing `ClassNotFoundException` and `getResource()` returning `null` after release verifies the actual close semantics rather than just cache eviction. That's the behavioral coverage I was asking for. 👍 Thanks also @DanielLeens for re-checking the runtime path. Agreed on your point: the catch block in `disableJarUrlCache()` does not override the deep-clean flag — the property is read independently, so a cache-disable failure won't silently flip deep-clean off. Good to have that confirmed on the record. Two remaining asks before I can approve: 1. **Docs (still open from my earlier review):** `SEATUNNEL_CLASSLOADER_DEEP_CLEAN` is a user-facing switch and currently undocumented. Please add a short section covering: what it does, that it's off by default, and — critically — that on JDK 9+ the reflective `URLClassPath`/JarLoader cache cleanup requires `--add-opens java.base/jdk.internal.loader=ALL-UNNAMED` (or the behavior degrades gracefully). The log reminder is nice, but users shouldn't have to discover this from logs. 2. **Re-review of the new test commit:** the test rework was pushed after my last pass, so I'll take one more look at the head to confirm the dynamic-JAR tests are stable in CI (runtime compilation can be flaky across JDK vendors). If CI stays green, no further action needed from you on this point. Once the docs are in, I'm happy to approve. Thanks both for the thorough back-and-forth — the reflection fallback behavior and the test rigor ended up in a much better place than where this PR started. <!-- streview-comment:91 --> -- 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]
