New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-34087][FOLLOW-UP][SQL] Manage ExecutionListenerBus register inside itself #31919
Conversation
private def registerForCleanup(objectForCleanup: AnyRef, task: CleanupTask): Unit = { | ||
referenceBuffer.add(new CleanupTaskWeakReference(task, objectForCleanup, referenceQueue)) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @Ngone51 . Could you revert the change in this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @dongjoon-hyun, this change intends to let the newly added registerSparkListenerForCleanup()
follow the same placement with other register...ForCleanup
methods. This would make the ContextCleaner
looks neater.
This is a style mistake made in the original PR so I fixed it in this follow-up PR. Does it sound ok to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it.
sql/core/src/main/scala/org/apache/spark/sql/util/QueryExecutionListener.scala
Outdated
Show resolved
Hide resolved
cc @mridulm |
This PR addressed the comment from https://github.com/apache/spark/pull/31881/files#r597370114 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks fine to me
Kubernetes integration test starting |
Kubernetes integration test status failure |
Test build #136325 has finished for PR 31919 at commit
|
Kubernetes integration test starting |
Kubernetes integration test status failure |
Test build #136333 has finished for PR 31919 at commit
|
dfd7d38
to
ae5d5d6
Compare
Kubernetes integration test starting |
Kubernetes integration test status failure |
Test build #136382 has finished for PR 31919 at commit
|
GA passed, merging to master, thanks! |
Thanks all! |
Late LGTM, thanks for fixing this @Ngone51 ! |
What changes were proposed in this pull request?
Move
ExecutionListenerBus
register (bothListenerBus
andContextCleaner
register) into itself.Also with a minor change that put
registerSparkListenerForCleanup
to a better place.Why are the changes needed?
improve code
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Pass existing tests.