nzw921rx commented on PR #10678:
URL: https://github.com/apache/seatunnel/pull/10678#issuecomment-4168385549
> @nzw921rx Thanks for the review and suggestions!
>
> I completely agree that an explicitly opt-in feature should have good
visibility.
>
> However, this PR is currently just **Phase 1**. The core goal is to fill
in the missing `URLClassLoader.close()` to establish a basic lifecycle
management mechanism, rather than striving for 100% thorough cleanup right now.
There are still massive strong references in the system (TCCL, JDBC
DriverManager, static caches, etc.), which will be systematically addressed in
the upcoming Phase 3 (see #10669).
>
> Because the overall feature is not yet fully closed-loop, even with
`DEEP_CLEAN` enabled, there will inevitably be some cleanup failures in
real-world environments. If we elevate all individual resource cleanup failures
to `WARN` right now, it would easily generate a massive amount of log noise and
cause unnecessary confusion for users.
>
> My proposed compromise for Phase 1 is as follows:
>
> * **Configuration/Initialization errors** (e.g., missing `--add-opens`) →
Elevate to `WARN`
> * **Deep clean successfully completed overall** → Print an `INFO` log
> * **Individual resource cleanup failures** → Keep at `DEBUG` level
>
> Once the strong reference issues are systematically resolved in later
phases and the feature becomes a true closed-loop, we can further refine the
logging strategy together.
>
> Do you think this approach is reasonable?
thanks @knight6236 reply
I agree that this approach is reasonable — there's no need to elevate all
log levels to WARNING, as that would only add noise and confuse users. For logs
that serve purely debugging purposes, I'd suggest removing some of the verbose
DEBUG statements and instead increasing unit test coverage to verify the
expected behavior.
example:
```
log.debug("Closed` and removed JarFile from JarFileFactory fileCache: {}",
`jarFile.getName());
```
These kinds of debug logs are certainly helpful during development, but
their practical value in production is relatively limited. I'd suggest
considering removing these DEBUG logs and instead covering the cleanup logic
with unit tests — this would provide more reliable and repeatable verification
while keeping the log output cleaner.
--
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]
