-
Notifications
You must be signed in to change notification settings - Fork 28.1k
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-5411] Allow SparkListeners to be specified in SparkConf and loaded when creating SparkContext #4111
[SPARK-5411] Allow SparkListeners to be specified in SparkConf and loaded when creating SparkContext #4111
Changes from 5 commits
163ba19
25988f3
217ecc0
9c0d8f1
b22b379
d0d276d
d6f3113
b9973da
2daff9b
1a5b9a0
6e0122c
8370839
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 |
---|---|---|
|
@@ -379,6 +379,41 @@ class SparkContext(config: SparkConf) extends Logging with ExecutorAllocationCli | |
} | ||
executorAllocationManager.foreach(_.start()) | ||
|
||
// Use reflection to instantiate listeners specified via the `spark.extraListeners` configuration | ||
// or the SPARK_EXTRA_LISTENERS environment variable | ||
try { | ||
val listenerClassNames: Seq[String] = { | ||
val fromSparkConf = conf.get("spark.extraListeners", "").split(',') | ||
val fromEnvVar = Option(conf.getenv("SPARK_EXTRA_LISTENERS")).getOrElse("").split(',') | ||
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. I thought we were moving away from adding new env-based config options. 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. This functions slightly differently than the rest of our env-based configurations: the settings from this configuration are merged with the SparkConf configuration, rather than overriding/being-overriden by it. My motivation for this configuration was to provide a mechanism for the execution environment to inject custom listeners without having to worry about them being overriden by settings in the user's SparkConf (this is necessary because we don't have a mechanism for automatic merging of Spark configurations). 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. I see. I think at some point it would be nice to have a better long-term solution for this kind of scenario. I added Anyway, separate discussion. 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. Hm - I'm also skeptical about having an env var here. This same reasoning could be used to argue having an env var for every conf value right (which we definitely don't want). @JoshRosen at least for the use case I had in mind for this, we could on our own preprocess the conf files to deal with merging (i.e. no need to put extra logic in Spark). |
||
(fromSparkConf ++ fromEnvVar).map(_.trim).filter(_ != "") | ||
} | ||
for (className <- listenerClassNames) { | ||
val listenerClass = Class.forName(className).asInstanceOf[Class[_ <: SparkListener]] | ||
val listener = try { | ||
listenerClass.getConstructor(classOf[SparkConf]).newInstance(conf) | ||
} catch { | ||
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. So using exceptions for basically branching is not really great. You can alternatively use: 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. Sorry, i meant getConstructors() (not getDeclaredConstructors) |
||
case e: NoSuchMethodException => | ||
try { | ||
listenerClass.newInstance() | ||
} catch { | ||
case e: NoSuchMethodException => | ||
throw new SparkException( | ||
s"$listenerClass did not have a zero-argument constructor or a" + | ||
" single-argument constructor that accepts SparkConf (is it a nested Scala class?)") | ||
} | ||
} | ||
listenerBus.addListener(listener) | ||
logInfo(s"Registered listener $listenerClass") | ||
} | ||
} catch { | ||
case e: Exception => | ||
try { | ||
stop() | ||
} finally { | ||
throw new SparkException(s"Exception when registering SparkListener", e) | ||
} | ||
} | ||
|
||
// At this point, all relevant SparkListeners have been registered, so begin releasing events | ||
listenerBus.start() | ||
|
||
|
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.
Nit: Can we group this logic with listenerBus.start() into a separate method called "setupListenerBus()" and call it in the constructor? The reason is this logic has to occur before the listenerBus.start() and having them grouped in a method can better ensure the order is preserved.