You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by vanzin <gi...@git.apache.org> on 2017/12/20 20:30:05 UTC
[GitHub] spark pull request #20039: [SPARK-22850][core] Ensure queued events are deli...
GitHub user vanzin opened a pull request:
https://github.com/apache/spark/pull/20039
[SPARK-22850][core] Ensure queued events are delivered to all event queues.
The code in LiveListenerBus was queueing events before start in the
queues themselves; so in situations like the following:
bus.post(someEvent)
bus.addToEventLogQueue(listener)
bus.start()
"someEvent" would not be delivered to "listener" if that was the first
listener in the queue, because the queue wouldn't exist when the
event was posted.
This change buffers the events before starting the bus in the bus itself,
so that they can be delivered to all registered queues when the bus is
started.
Also tweaked the unit tests to cover the behavior above.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/vanzin/spark SPARK-22850
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/20039.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #20039
----
commit 80b900a3e4d26cc982df914ece79e5435b7ff5df
Author: Marcelo Vanzin <va...@...>
Date: 2017-12-20T20:17:43Z
[SPARK-22850][core] Ensure queued events are delivered to all event queues.
The code in LiveListenerBus was queueing events before start in the
queues themselves; so in situations like the following:
bus.post(someEvent)
bus.addToEventLogQueue(listener)
bus.start()
"someEvent" would not be delivered to "listener" if that was the first
listener in the queue.
This change buffers the events before starting the bus in the bus itself,
so that they can be delivered to all registered queues when the bus is
started.
Also tweaked the unit tests to cover the behavior above.
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20039: [SPARK-22850][core] Ensure queued events are deli...
Posted by Ngone51 <gi...@git.apache.org>.
Github user Ngone51 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20039#discussion_r159081427
--- Diff: core/src/main/scala/org/apache/spark/scheduler/LiveListenerBus.scala ---
@@ -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)
--- End diff --
Okay, perfect explanation. Thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20039: [SPARK-22850][core] Ensure queued events are delivered t...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20039
**[Test build #85258 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85258/testReport)** for PR 20039 at commit [`80b900a`](https://github.com/apache/spark/commit/80b900a3e4d26cc982df914ece79e5435b7ff5df).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20039: [SPARK-22850][core] Ensure queued events are delivered t...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/20039
retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20039: [SPARK-22850][core] Ensure queued events are deli...
Posted by Ngone51 <gi...@git.apache.org>.
Github user Ngone51 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20039#discussion_r158572288
--- Diff: core/src/main/scala/org/apache/spark/scheduler/LiveListenerBus.scala ---
@@ -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)
--- End diff --
Yes，that‘s true, semantic only.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20039: [SPARK-22850][core] Ensure queued events are deli...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20039#discussion_r158544313
--- Diff: core/src/main/scala/org/apache/spark/scheduler/LiveListenerBus.scala ---
@@ -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
+ }
--- End diff --
Let me see if I can rearrange things to avoid the extra synchronization...
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20039: [SPARK-22850][core] Ensure queued events are delivered t...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20039
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/85206/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20039: [SPARK-22850][core] Ensure queued events are delivered t...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on the issue:
https://github.com/apache/spark/pull/20039
ahhhh, got it. sorry I misunderstood the comments on the jira ... I thought your second comment meant the original description was incorrect.
anyway -- lgtm
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20039: [SPARK-22850][core] Ensure queued events are delivered t...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/20039
#20038
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20039: [SPARK-22850][core] Ensure queued events are deli...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on a diff in the pull request:
https://github.com/apache/spark/pull/20039#discussion_r159075179
--- Diff: core/src/main/scala/org/apache/spark/scheduler/LiveListenerBus.scala ---
@@ -125,13 +128,39 @@ private[spark] class LiveListenerBus(conf: SparkConf) {
/** Post an event to all queues. */
def post(event: SparkListenerEvent): Unit = {
- if (!stopped.get()) {
- metrics.numEventsPosted.inc()
- val it = queues.iterator()
- while (it.hasNext()) {
- it.next().post(event)
+ if (stopped.get()) {
+ return
+ }
+
+ metrics.numEventsPosted.inc()
+
+ // If the event buffer is null, it means the bus has been started and we can avoid
+ // synchronization and post events directly to the queues. This should be the most
+ // common case during the life of the bus.
+ if (queuedEvents == null) {
+ postToQueues(event)
--- End diff --
I don't think this would help at all. IIUC, you're saying that its possible that a `post()` and a `stop()` are racing against each other, and we might post to a queue after its been stopped. But thats OK -- the queues are prepared to deal with this.
Its also possible that during that race, the event makes it one queue before that queue is stopped, but to another queue after its stopped. Which means that final event only makes it to some queues. Again, I think that is fine, and your change wouldn't help anyway, that would still be possible.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20039: [SPARK-22850][core] Ensure queued events are delivered t...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20039
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/85317/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20039: [SPARK-22850][core] Ensure queued events are delivered t...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20039
**[Test build #85206 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85206/testReport)** for PR 20039 at commit [`80b900a`](https://github.com/apache/spark/commit/80b900a3e4d26cc982df914ece79e5435b7ff5df).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20039: [SPARK-22850][core] Ensure queued events are delivered t...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/20039
> where is the SparkListenerExecutorAdded event for the driver?
There isn't one. There's just SparkListenerBlockManagerAdded. But the old executor list was based on block managers, not executor added / removed, so it included the driver.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20039: [SPARK-22850][core] Ensure queued events are delivered t...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20039
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20039: [SPARK-22850][core] Ensure queued events are deli...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20039#discussion_r158333948
--- Diff: core/src/main/scala/org/apache/spark/scheduler/LiveListenerBus.scala ---
@@ -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
+ }
--- End diff --
It avoids having to indent the rest of the code.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20039: [SPARK-22850][core] Ensure queued events are deli...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on a diff in the pull request:
https://github.com/apache/spark/pull/20039#discussion_r159074536
--- Diff: core/src/main/scala/org/apache/spark/scheduler/LiveListenerBus.scala ---
@@ -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)
--- End diff --
both `start()` and `stop()` are synchronized so they can't be interleaved.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20039: [SPARK-22850][core] Ensure queued events are delivered t...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20039
**[Test build #85285 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85285/testReport)** for PR 20039 at commit [`80b900a`](https://github.com/apache/spark/commit/80b900a3e4d26cc982df914ece79e5435b7ff5df).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20039: [SPARK-22850][core] Ensure queued events are delivered t...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20039
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/85285/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20039: [SPARK-22850][core] Ensure queued events are delivered t...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on the issue:
https://github.com/apache/spark/pull/20039
this change looks fine -- but I'm having trouble connecting this to the original bug. I guess really I'm confused about how driver logs ever worked ... where is the SparkListenerExecutorAdded event for the driver?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20039: [SPARK-22850][core] Ensure queued events are delivered t...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20039
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20039: [SPARK-22850][core] Ensure queued events are deli...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20039#discussion_r158538406
--- Diff: core/src/main/scala/org/apache/spark/scheduler/LiveListenerBus.scala ---
@@ -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
+ }
--- End diff --
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 after `start()` is called.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20039: [SPARK-22850][core] Ensure queued events are deli...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20039#discussion_r158538462
--- Diff: core/src/main/scala/org/apache/spark/scheduler/LiveListenerBus.scala ---
@@ -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)
--- End diff --
That really does not make any difference in behavior.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20039: [SPARK-22850][core] Ensure queued events are delivered t...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20039
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20039: [SPARK-22850][core] Ensure queued events are deli...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on a diff in the pull request:
https://github.com/apache/spark/pull/20039#discussion_r159080455
--- Diff: core/src/main/scala/org/apache/spark/scheduler/LiveListenerBus.scala ---
@@ -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)
--- End diff --
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 `start()`, switch to `stop()` and get a few lines in ... but then `stop()` gets blocked on `start()` anyway. Sure, that means that while `start()` is running, we also have `stopped == true` ... but so what? More `post()` calls will be no-ops. `start()` will start the queues and post all buffered events, then release the lock and `stop()` will stop all the queues.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20039: [SPARK-22850][core] Ensure queued events are deli...
Posted by gczsjdy <gi...@git.apache.org>.
Github user gczsjdy commented on a diff in the pull request:
https://github.com/apache/spark/pull/20039#discussion_r158309818
--- Diff: core/src/main/scala/org/apache/spark/scheduler/LiveListenerBus.scala ---
@@ -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
+ }
--- End diff --
What's the difference between this and the original way
```
if (!stopped.get()) {
...
}
```
?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20039: [SPARK-22850][core] Ensure queued events are delivered t...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20039
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/85258/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20039: [SPARK-22850][core] Ensure queued events are deli...
Posted by Ngone51 <gi...@git.apache.org>.
Github user Ngone51 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20039#discussion_r158574199
--- Diff: core/src/main/scala/org/apache/spark/scheduler/LiveListenerBus.scala ---
@@ -125,13 +128,39 @@ private[spark] class LiveListenerBus(conf: SparkConf) {
/** Post an event to all queues. */
def post(event: SparkListenerEvent): Unit = {
- if (!stopped.get()) {
- metrics.numEventsPosted.inc()
- val it = queues.iterator()
- while (it.hasNext()) {
- it.next().post(event)
+ if (stopped.get()) {
+ return
+ }
+
+ metrics.numEventsPosted.inc()
+
+ // If the event buffer is null, it means the bus has been started and we can avoid
+ // synchronization and post events directly to the queues. This should be the most
+ // common case during the life of the bus.
+ if (queuedEvents == null) {
+ postToQueues(event)
--- End diff --
What if stop() called after the null judge and before postToQueues() call ?
Do you think we should check the stopped.get() in postToQueues()?
like:
```
private def postToQueues(event: SparkListenerEvent): Unit = {
if (!stopped.get()) {
val it = queues.iterator()
while (it.hasNext()) {
it.next().post(event)
}
}
}
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20039: [SPARK-22850][core] Ensure queued events are deli...
Posted by Ngone51 <gi...@git.apache.org>.
Github user Ngone51 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20039#discussion_r159076507
--- Diff: core/src/main/scala/org/apache/spark/scheduler/LiveListenerBus.scala ---
@@ -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)
--- End diff --
LiveListener's stop() is not synchronized completely. And LiveListener's `stopped` variable could be set "true" in stop() (before the synchronize body) while start() is calling. Do I missing something?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20039: [SPARK-22850][core] Ensure queued events are delivered t...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20039
**[Test build #85317 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85317/testReport)** for PR 20039 at commit [`2602fa6`](https://github.com/apache/spark/commit/2602fa68424e984f2cd49f79fb54bcf9676ba5fb).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20039: [SPARK-22850][core] Ensure queued events are delivered t...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20039
**[Test build #85285 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85285/testReport)** for PR 20039 at commit [`80b900a`](https://github.com/apache/spark/commit/80b900a3e4d26cc982df914ece79e5435b7ff5df).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20039: [SPARK-22850][core] Ensure queued events are delivered t...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on the issue:
https://github.com/apache/spark/pull/20039
that explains how it gets into the list of executors in the UI -- but where does it get the link for the logs?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20039: [SPARK-22850][core] Ensure queued events are delivered t...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on the issue:
https://github.com/apache/spark/pull/20039
merged to master / 2.3
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20039: [SPARK-22850][core] Ensure queued events are delivered t...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/20039
That failing test looks flaky. retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20039: [SPARK-22850][core] Ensure queued events are delivered t...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20039
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20039: [SPARK-22850][core] Ensure queued events are deli...
Posted by Ngone51 <gi...@git.apache.org>.
Github user Ngone51 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20039#discussion_r158573853
--- Diff: core/src/main/scala/org/apache/spark/scheduler/LiveListenerBus.scala ---
@@ -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)
--- End diff --
Ok，**synchronized** can avoid this problem.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20039: [SPARK-22850][core] Ensure queued events are deli...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/20039
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20039: [SPARK-22850][core] Ensure queued events are delivered t...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20039
**[Test build #85206 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85206/testReport)** for PR 20039 at commit [`80b900a`](https://github.com/apache/spark/commit/80b900a3e4d26cc982df914ece79e5435b7ff5df).
* This patch **fails PySpark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20039: [SPARK-22850][core] Ensure queued events are delivered t...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20039
**[Test build #85317 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85317/testReport)** for PR 20039 at commit [`2602fa6`](https://github.com/apache/spark/commit/2602fa68424e984f2cd49f79fb54bcf9676ba5fb).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20039: [SPARK-22850][core] Ensure queued events are deli...
Posted by Ngone51 <gi...@git.apache.org>.
Github user Ngone51 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20039#discussion_r158573360
--- Diff: core/src/main/scala/org/apache/spark/scheduler/LiveListenerBus.scala ---
@@ -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)
--- End diff --
What if stop() called before all queuedEvents post to AsyncEventQueue?
```
/**
* Stop the listener bus. It will wait until the queued events have been processed, but new
* events will be dropped.
*/
```
**(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?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20039: [SPARK-22850][core] Ensure queued events are deli...
Posted by gczsjdy <gi...@git.apache.org>.
Github user gczsjdy commented on a diff in the pull request:
https://github.com/apache/spark/pull/20039#discussion_r158424211
--- Diff: core/src/main/scala/org/apache/spark/scheduler/LiveListenerBus.scala ---
@@ -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
+ }
--- End diff --
Is `synchronized` needed only due to the modification of `queuedEvents`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20039: [SPARK-22850][core] Ensure queued events are delivered t...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20039
**[Test build #85258 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85258/testReport)** for PR 20039 at commit [`80b900a`](https://github.com/apache/spark/commit/80b900a3e4d26cc982df914ece79e5435b7ff5df).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20039: [SPARK-22850][core] Ensure queued events are deli...
Posted by Ngone51 <gi...@git.apache.org>.
Github user Ngone51 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20039#discussion_r158424923
--- Diff: core/src/main/scala/org/apache/spark/scheduler/LiveListenerBus.scala ---
@@ -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)
--- End diff --
```
q.start(sc)
queuedEvents.foreach(q.post)
```
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org