[SPARK-15703] [Scheduler][Core][WebUI] Make ListenerBus event queue size configurable #14269

dhruve · 2016-07-19T18:09:21Z

What changes were proposed in this pull request?

This change adds a new configuration entry to specify the size of the spark listener bus event queue. The value for this config ("spark.scheduler.listenerbus.eventqueue.size") is set to a default to 10000.

Note:
I haven't currently documented the configuration entry. We can decide whether it would be appropriate to make it a public configuration or keep it as an undocumented one. Refer JIRA for more details.

How was this patch tested?

Ran existing jobs and verified the event queue size with debug logs and from the Spark WebUI Environment tab.

tgravescs · 2016-07-19T18:12:55Z

Ok to test

tgravescs · 2016-07-19T18:23:00Z

Jenkins, test this please

tgravescs · 2016-07-19T19:27:53Z

core/src/main/scala/org/apache/spark/scheduler/LiveListenerBus.scala

-  private val EVENT_QUEUE_CAPACITY = 10000
-  private val eventQueue = new LinkedBlockingQueue[SparkListenerEvent](EVENT_QUEUE_CAPACITY)
+  private lazy val EVENT_QUEUE_CAPACITY = sparkContext.conf.
+    getInt("spark.scheduler.listenerbus.eventqueue.size", 10000)


should use a ConfigEntry for this

SparkQA · 2016-07-19T20:11:06Z

Test build #62548 has finished for PR 14269 at commit 9c0cb44.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

…e tests

tgravescs · 2016-07-20T15:55:22Z

Jenkins, this is okay to test

tgravescs · 2016-07-20T16:24:33Z

Jenkins, test this please

tgravescs · 2016-07-20T17:46:04Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

+  private[spark] val LISTENER_BUS_EVENT_QUEUE_SIZE =
+    ConfigBuilder("spark.scheduler.listenerbus.eventqueue.size")
+      .intConf
+      .transform((x: Int) => {


The transform wasn't really meant to be a validator. We had discussed adding validator functionality to the Config stuff but I think that should be done separately. So for now lets pull this out and just do the check in the LiveListenerBus.

Agreed. The method is intended for something else. Having the validator functionality here would be good to do so. I will make the changes and fix the tests.

SparkQA · 2016-07-20T18:01:57Z

Test build #62613 has finished for PR 14269 at commit 76d9af8.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

tgravescs · 2016-07-21T14:52:16Z

Jenkins, test this please

dhruve · 2016-07-21T21:08:57Z

Jenkins, okay to test

dhruve · 2016-07-22T14:51:04Z

@vanzin, @srowen Can you kick of the Jenkins build? Thanks.

vanzin · 2016-07-22T16:51:58Z

ok to test

vanzin · 2016-07-22T16:52:08Z

retest this please

SparkQA · 2016-07-22T18:33:21Z

Test build #62733 has finished for PR 14269 at commit 889fe66.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-07-22T18:44:56Z

Test build #62732 has finished for PR 14269 at commit 889fe66.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dhruve · 2016-07-22T20:30:57Z

The set of failures from Test build #62733 are unrelated.

tgravescs · 2016-07-25T20:58:21Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

@@ -19,6 +19,7 @@ package org.apache.spark.internal

 import java.util.concurrent.TimeUnit

+import org.apache.spark.SparkException


remove I think this is unused now.

tgravescs · 2016-07-25T21:03:25Z

one minor issue with import otherwise changes LGTM.

dhruve · 2016-07-25T21:07:22Z

Removed unused import.

SparkQA · 2016-07-25T23:15:17Z

Test build #62842 has finished for PR 14269 at commit 82feec4.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

…ze configurable (branch 2.0) ## What changes were proposed in this pull request? Backport #14269 to 2.0. ## How was this patch tested? Jenkins. Author: Dhruve Ashar <dhruveashar@gmail.com> Closes #15222 from zsxwing/SPARK-15703-2.0.

JoshRosen · 2017-05-23T21:04:42Z

core/src/main/scala/org/apache/spark/scheduler/LiveListenerBus.scala

@@ -32,18 +33,24 @@ import org.apache.spark.util.Utils
 * has started will events be actually propagated to all attached listeners. This listener bus
 * is stopped when `stop()` is called, and it will drop further events after stopping.
 */
-private[spark] class LiveListenerBus extends SparkListenerBus {
+private[spark] class LiveListenerBus(val sparkContext: SparkContext) extends SparkListenerBus {


I'm modifying LiveListenerBus now and noticed that we're passing in sparkContext even though we only use it to access conf. I think it would have been better to just pass in conf here. It would make the initialization order constraints a lot clearer, too: right now it's not immediately clear why eventQueue needs to be a lazy val.

I guess we also use this to tear down sparkContext when the listener dies. I still have a weak preference for the old code, however, since I think the lifecycle was clearer.

From what I recall and as you mentioned, a ref to sparkContext was already being used to tear down when the listener died because of an uncaught Exception. The idea was to refactor the code to make sparkContext available at instantiation time and access the conf from it rather than passing it separately. This defn is cyclic and lazy val was used to ensure that conf was accessed only after sc was intialized.

JoshRosen · 2017-05-23T21:41:50Z

streaming/src/test/scala/org/apache/spark/streaming/ReceivedBlockHandlerSuite.scala

@@ -47,6 +47,7 @@ class ReceivedBlockHandlerSuite
  extends SparkFunSuite
  with BeforeAndAfter
  with Matchers
+  with LocalSparkContext


The fact that this change was necessary is another weird code smell which suggests to me that putting SparkContext into the constructor was not a good idea.

JoshRosen · 2017-05-25T07:22:04Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

@@ -103,4 +103,9 @@ package object config {
    .stringConf
    .checkValues(Set("hive", "in-memory"))
    .createWithDefault("in-memory")
+
+  private[spark] val LISTENER_BUS_EVENT_QUEUE_SIZE =
+    ConfigBuilder("spark.scheduler.listenerbus.eventqueue.size")


Another post-hoc review/complaint: I think that size might be misleading in this context where we're talking about a queue, since the size of a queue refers to the number of elements currently in the queue while its capacity refers to the maximum size that the queue can reach. This configuration name caused confusion in https://github.com/apache/spark/pull/18083/files/378206efb9f5c9628a678ba7defb536252f5cbcb#r118413115

Instead, it might have been better to call it capacity.

I agree. Capacity would have been a better choice.

## What changes were proposed in this pull request? `ConfigBuilder` builds `ConfigEntry` which can only read value with one key, if we wanna change the config name but still keep the old one, it's hard to do. This PR introduce `ConfigBuilder.withAlternative`, to support reading config value with alternative keys. And also rename `spark.scheduler.listenerbus.eventqueue.size` to `spark.scheduler.listenerbus.eventqueue.capacity` with this feature, according to #14269 (comment) ## How was this patch tested? a new test Author: Wenchen Fan <wenchen@databricks.com> Closes #18110 from cloud-fan/config.

[SPARK-15703] Make ListenerBus event queue configurable

9c0cb44

tgravescs reviewed Jul 19, 2016
View reviewed changes

[SPARK-15703] Make ListenerBus event queue size ConfigEntry and updat…

76d9af8

…e tests

tgravescs reviewed Jul 20, 2016
View reviewed changes

dhruve added 2 commits July 20, 2016 16:49

[SPARK-15703] Fixes test failures to ensure single sc

09e855e

[SPARK-15703] Fixes import ordering

41dc57c

Remove extra line

889fe66

tgravescs reviewed Jul 25, 2016
View reviewed changes

[SPARK-15703] Remove unused import

82feec4

asfgit closed this in 0b71d9a Jul 26, 2016

zsxwing mentioned this pull request Sep 23, 2016

[SPARK-15703][SCHEDULER][CORE][WEBUI] Make ListenerBus event queue size configurable (branch 2.0) #15222

Closed

JoshRosen reviewed May 23, 2017

View reviewed changes

JoshRosen mentioned this pull request May 24, 2017

[SPARK-20863] Add metrics/instrumentation to LiveListenerBus #18083

Closed

JoshRosen reviewed May 25, 2017

View reviewed changes

cloud-fan mentioned this pull request May 25, 2017

[SPARK-20887][CORE] support alternative keys in ConfigBuilder #18110

Closed

		@@ -19,6 +19,7 @@ package org.apache.spark.internal

		import java.util.concurrent.TimeUnit

		import org.apache.spark.SparkException

[SPARK-15703] [Scheduler][Core][WebUI] Make ListenerBus event queue size configurable #14269

[SPARK-15703] [Scheduler][Core][WebUI] Make ListenerBus event queue size configurable #14269

Uh oh!

Conversation

dhruve commented Jul 19, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

tgravescs commented Jul 19, 2016

Uh oh!

tgravescs commented Jul 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 19, 2016

Uh oh!

tgravescs commented Jul 20, 2016

Uh oh!

tgravescs commented Jul 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 20, 2016

Uh oh!

tgravescs commented Jul 21, 2016

Uh oh!

dhruve commented Jul 21, 2016

Uh oh!

dhruve commented Jul 22, 2016

Uh oh!

vanzin commented Jul 22, 2016

Uh oh!

vanzin commented Jul 22, 2016

Uh oh!

SparkQA commented Jul 22, 2016

Uh oh!

SparkQA commented Jul 22, 2016

Uh oh!

dhruve commented Jul 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgravescs commented Jul 25, 2016

Uh oh!

dhruve commented Jul 25, 2016

Uh oh!

SparkQA commented Jul 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoshRosen May 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JoshRosen May 25, 2017 •

edited

Loading