-
Notifications
You must be signed in to change notification settings - Fork 28.5k
[SPARK-51981][SS] Add JobTags to queryStartedEvent #50780
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -288,7 +288,14 @@ abstract class StreamExecution( | |||
// `postEvent` does not throw non fatal exception. | ||||
val startTimestamp = triggerClock.getTimeMillis() | ||||
postEvent( | ||||
new QueryStartedEvent(id, runId, name, progressReporter.formatTimestamp(startTimestamp))) | ||||
new QueryStartedEvent( | ||||
id, | ||||
runId, | ||||
name, | ||||
progressReporter.formatTimestamp(startTimestamp), | ||||
sparkSession.sparkContext.getJobTags() | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How many elements are we anticipating to have here? The size of event should be considerably small enough. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah it should be quite small, we already have the job tags in other listener event already, for example here spark/sql/core/src/main/scala/org/apache/spark/sql/execution/ui/SQLListener.scala Line 57 in f643b64
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for explanation. |
||||
) | ||||
) | ||||
|
||||
// Unblock starting thread | ||||
startLatch.countDown() | ||||
|
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.
no biggie but you can call
set(jobTags().toList())
which will be automatically a Python list. Having individual Py4J call is actually pretty expensive. But tags are supposed to be few so I don't mind it. leave it to you with my approval.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.
I tried, but it didn't works. See the comment I put above #50780 (comment). I don't know why though, this is the tests result I got when running locally
Could this be a issue with regarding to package version etc?
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.
ah okie that's fine
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.
Sg, thanks! Can you help me merge this PR once the tests pass?