-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-22850][core] Ensure queued events are delivered to all event queues. #20039
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 1 commit
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 |
|---|---|---|
|
|
@@ -62,6 +62,9 @@ private[spark] class LiveListenerBus(conf: SparkConf) { | |
|
|
||
| private val queues = new CopyOnWriteArrayList[AsyncEventQueue]() | ||
|
|
||
| // Visible for testing. | ||
| private[scheduler] var queuedEvents = new mutable.ListBuffer[SparkListenerEvent]() | ||
|
|
||
| /** Add a listener to queue shared by all non-internal listeners. */ | ||
| def addToSharedQueue(listener: SparkListenerInterface): Unit = { | ||
| addToQueue(listener, SHARED_QUEUE) | ||
|
|
@@ -124,13 +127,19 @@ private[spark] class LiveListenerBus(conf: SparkConf) { | |
| } | ||
|
|
||
| /** Post an event to all queues. */ | ||
| def post(event: SparkListenerEvent): Unit = { | ||
| if (!stopped.get()) { | ||
| metrics.numEventsPosted.inc() | ||
| def post(event: SparkListenerEvent): Unit = synchronized { | ||
| if (stopped.get()) { | ||
| return | ||
| } | ||
|
|
||
| metrics.numEventsPosted.inc() | ||
| if (started.get()) { | ||
| val it = queues.iterator() | ||
| while (it.hasNext()) { | ||
| it.next().post(event) | ||
| } | ||
| } else { | ||
| queuedEvents += event | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -149,7 +158,11 @@ private[spark] class LiveListenerBus(conf: SparkConf) { | |
| } | ||
|
|
||
| this.sparkContext = sc | ||
| queues.asScala.foreach(_.start(sc)) | ||
| queues.asScala.foreach { q => | ||
| q.start(sc) | ||
| queuedEvents.foreach(q.post) | ||
|
Member
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. Ummm... In my opinion, exchange these two lines sequence would be better for following the original logic of events buffered before a queue calls start(). So, queuedEvents post to queues first before queues start would be unified logically.
Contributor
Author
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. That really does not make any difference in behavior.
Member
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. What if stop() called before all queuedEvents post to AsyncEventQueue? (the "queued events" mentioned in description above is not equal to "queuedEvents" here.) As queuedEvents "post" before listeners install, so, can they be treated as new events?
Contributor
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. both
Member
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. LiveListener's stop() is not synchronized completely. And LiveListener's
Contributor
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. sure, but the interleaving you were worried about specifically above isn't possible because of the synchronized blocks. Yes, you could get one line into
Member
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. Okay, perfect explanation. Thanks. |
||
| } | ||
| queuedEvents = null | ||
| metricsSystem.registerSource(metrics) | ||
| } | ||
|
|
||
|
|
||
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.
What's the difference between this and the original way
?
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.
It avoids having to indent the rest of the code.
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.
Is
synchronizedneeded only due to the modification ofqueuedEvents?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. It's needed because you have to make sure that when the buffered events are posted to the queues in
start(), this method cannot be called. Otherwise you need to have a contract that this class is not thread-safe until afterstart()is called.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.
Let me see if I can rearrange things to avoid the extra synchronization...