[SPARK-48370][CONNECT][FOLLOW-UP] Use JDK's Cleaner instead #46726
Conversation
| } | ||
| } catch { | ||
| case e: Throwable => logError("Error in cleaning thread", e) |
There was a problem hiding this comment.
The error is swollen but I think it's better to explicitly log
|
cc @hvanhovell |
| } | ||
| cleaningThread.join() | ||
| } | ||
| private val cleaner = Cleaner.create() |
There was a problem hiding this comment.
It seems that the lifecycle of SessionCleaner is the same as SparkSession, so when the client holds multiple SparkSessions, multiple instances of java.lang.ref.Cleaner will be created. If cleaner is defined in the companion object of SessionCleaner, it can allow multiple SessionCleaner to share one java.lang.ref.Cleaner instance . Can this meet the requirements?
There was a problem hiding this comment.
We can have one and share between other sessions but wanted to scope the cleaning specific to a session so it doesn't affect other sessions. I am fine either way though.
There was a problem hiding this comment.
We can do this in a follow-up? @LuciferYang is there any concrete concern here? Or are you just being tidy?
There was a problem hiding this comment.
It's fine, we can make follow-up when this really becomes an issue :)
|
Merged to master. |
### What changes were proposed in this pull request? This PR is a followup of apache#46683 that replaces our custom cleaner to JDK's cleaner. ### Why are the changes needed? Reuse the standard builtin library. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? I manually tested via reenabling `CheckpointSuite.checkpoint gc derived DataFrame` ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#46726 from HyukjinKwon/SPARK-48370-followup. Authored-by: Hyukjin Kwon <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>
What changes were proposed in this pull request?
This PR is a followup of #46683 that replaces our custom cleaner to JDK's cleaner.
Why are the changes needed?
Reuse the standard builtin library.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
I manually tested via reenabling
CheckpointSuite.checkpoint gc derived DataFrameWas this patch authored or co-authored using generative AI tooling?
No.