SEZ9 commented on PR #10678: URL: https://github.com/apache/seatunnel/pull/10678#issuecomment-5230719796
Thanks @DanielLeens for the thorough re-verification, and thanks @knight6236 for the patience here. I've re-checked the discussion points against head `89ff5c3f63c2`: **Resolved from my side:** - **Catch block vs. deep-clean flag**: Confirmed — the failure path in `disableJarUrlCache()` does not override `DEEP_CLEAN_ENABLED`, since the flag is read independently from the system property (lines 77-80). Thanks for closing that loop. - **`--add-opens` awareness**: The log reminder that fires only when deep clean is explicitly enabled is a reasonable mitigation, given the feature is opt-in and off by default. Users who enable it get a clear signal about the JVM flag requirement. **Remaining asks before I approve:** 1. **Docs** (my original Issue 1, still open): please add the `SEATUNNEL_CLASSLOADER_DEEP_CLEAN` system property and the required `--add-opens java.base/jdk.internal.loader=ALL-UNNAMED` flag to the deployment docs. Since this is a user-facing switch with a JVM flag prerequisite, a log message alone isn't discoverable enough. 2. **Graceful reflection failure on JDK 9+**: please confirm (or add a test showing) that `clearUrlClassPathCache()`/`closeJarLoader()` catch `InaccessibleObjectException` and `NoSuchFieldException` and degrade to a warning rather than propagating, so enabling deep clean on JDK 11/17 without the flag doesn't break job cleanup. 3. **Static flag semantics**: since `DEEP_CLEAN_ENABLED` is read per-constructor into static state, a second service instance in the same JVM can flip behavior for the first. Either make it read-once (e.g., a static final holder) or add a code comment documenting the intended semantics — a small change, but worth being deliberate about. None of these require redesign — 2 and 3 may already be satisfied by the current code, in which case a quick pointer to the relevant lines is enough. Once the docs are in and 2/3 are confirmed, I'm good to approve. <!-- streview-comment:96 --> -- 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]
