knight6236 commented on PR #10678: URL: https://github.com/apache/seatunnel/pull/10678#issuecomment-5275774130
@DanielLeens Thank you all for the thorough reviews across multiple rounds. I appreciate the time and effort each of you has put into verifying the changes independently. I have reviewed all the remaining points and would like to respond to the two open items: --- 1. Documentation for SEATUNNEL_CLASSLOADER_DEEP_CLEAN This is a deliberate choice rather than an omission. The deep-clean feature is still in Phase 1/2 of a multi-phase plan. Phase 3, which removes the remaining strong references that keep classloaders reachable, is not yet complete. Until Phase 3 lands, the deep-clean path – while functional – may produce unexpected behavior in certain edge cases because the underlying reference graph is not yet fully cleaned. Given this, I intentionally deferred user-facing documentation to a later phase for the following reasons: · Premature documentation would encourage users to enable a feature that is not yet fully battle-tested, potentially leading to support issues. · The feature is off by default (SEATUNNEL_CLASSLOADER_DEEP_CLEAN=false), so no user will encounter it without explicitly opting in. · The WARN logs and code comments provide sufficient guidance for advanced users/developers who choose to experiment with it. My plan: When Phase 3 is completed and the feature reaches a stable, fully-tested state, I will submit a comprehensive documentation update covering: · The purpose of the switch · The two required --add-opens flags for JDK 9+ · The JDK 8 global cache behavior trade-off · Recommendations for production usage I have already updated the PR description to reflect this phased approach. The current PR focuses on making the code correct and safe; documentation will follow as a dedicated commit once the feature is fully ready for general adoption. --- 2. Runtime-compiled test JAR (testDeepCleanModeEnabled) This design is intentional and I consider it cleaner than the alternatives. The test compiles a small Java source file at runtime to produce a real JAR, then loads a class from it and verifies that releaseClassLoader() actually unloads the class (via ClassNotFoundException) and releases resources (via getResource() == null). I chose runtime compilation over a pre-compiled test JAR (i.e., committing a .jar binary to the repo) for the following reasons: · Binary bloat: A pre-compiled JAR would add unnecessary binary artifacts to the source tree, making code review harder and increasing repository size. · Version skew: A pre-compiled JAR would be frozen to whatever JDK version it was built with. If the test framework or compilation target changes, the pre-compiled binary may no longer be representative of the actual runtime environment, potentially masking real issues. · Bytecode compatibility: Pre-compiled binaries can become incompatible across JDK versions (e.g., class file format changes, removed APIs), requiring periodic regeneration and risking subtle test failures. · Transparency: The source code is self-contained within the test file, making it immediately obvious what the test JAR contains without needing to reverse-engineer a binary. Runtime compilation ensures the test artifact is always built with the exact same JDK that is running the test suite, which is the most accurate reflection of real user environments. Regarding the stability concern (potential flakiness across JDK vendors) – the test uses the standard javax.tools.JavaCompiler API, which is part of the JDK specification. All compliant JDK implementations (Corretto, OpenJDK, Oracle, etc.) provide a compatible implementation. The CI is currently green on this test. If any future flakiness arises, I will address it at that time – but pre-compiling the JAR would not be my first choice, as it would sacrifice the benefits listed above while not guaranteeing stability either. --- Summary of current status Item Status JDK 9+ --add-opens log/warning ✅ Fixed JDK 8 global cache – opt-in only ✅ Fixed Static flag → instance field ✅ Fixed Reflection failure graceful degradation ✅ Fixed + tested close() synchronized ✅ Fixed closeJarLoader() simplified ✅ Fixed Documentation ⏳ Deliberately deferred to Phase 3 Runtime-compiled test stability ✅ Stable in CI; monitoring --- I believe the current head (bbb9245454c04988f1669663522d9cd4bdd737e6) is now technically complete and addresses all the blockers raised in previous reviews. The documentation will follow in a subsequent PR once the feature reaches its final stable state. I am happy to re-review any concerns and would appreciate approval if the current state is acceptable. Thank you again for the rigorous and constructive review process – it has significantly improved the quality of this PR. -- 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]
