Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/433#discussion_r11786442
--- Diff:
yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -361,55 +361,48 @@ object ClientBase {
val LOG4J_CONF_ENV_KEY: String = "SPARK_LOG4J_CONF"
val LOCAL_SCHEME = "local"
- // Based on code from org.apache.hadoop.mapreduce.v2.util.MRApps
- def populateHadoopClasspath(conf: Configuration, env: HashMap[String,
String]) {
- val classpathEntries = Option(conf.getStrings(
- YarnConfiguration.YARN_APPLICATION_CLASSPATH)).getOrElse(
- getDefaultYarnApplicationClasspath())
- for (c <- classpathEntries) {
- YarnSparkHadoopUtil.addToEnvironment(env,
Environment.CLASSPATH.name, c.trim,
+ def populateHadoopClasspath(conf: Configuration, env: HashMap[String,
String]) = {
+ val classPathElementsToAdd = getYarnAppClasspath(conf) ++
getMRAppClasspath(conf)
+ for (c <- classPathElementsToAdd.flatten) {
+ YarnSparkHadoopUtil.addToEnvironment(
+ env,
+ Environment.CLASSPATH.name,
+ c.trim,
File.pathSeparator)
}
+ classPathElementsToAdd
+ }
- val mrClasspathEntries = Option(conf.getStrings(
- "mapreduce.application.classpath")).getOrElse(
- getDefaultMRApplicationClasspath())
- if (mrClasspathEntries != null) {
- for (c <- mrClasspathEntries) {
- YarnSparkHadoopUtil.addToEnvironment(env,
Environment.CLASSPATH.name, c.trim,
- File.pathSeparator)
- }
+ private def getYarnAppClasspath(conf: Configuration):
Option[Array[String]] =
+ Option(conf.getStrings(YarnConfiguration.YARN_APPLICATION_CLASSPATH))
match {
+ case s: Some[Array[String]] => s
+ case None => getDefaultYarnApplicationClasspath
}
- }
- def getDefaultYarnApplicationClasspath(): Array[String] = {
- try {
- val field =
classOf[MRJobConfig].getField("DEFAULT_YARN_APPLICATION_CLASSPATH")
- field.get(null).asInstanceOf[Array[String]]
- } catch {
- case err: NoSuchFieldError => null
- case err: NoSuchFieldException => null
+ private def getMRAppClasspath(conf: Configuration):
Option[Array[String]] =
+ Option(conf.getStrings("mapreduce.application.classpath")) match {
+ case s: Some[Array[String]] => s
+ case None => getDefaultMRApplicationClasspath
}
- }
+
+ def getDefaultYarnApplicationClasspath: Option[Array[String]] =
Try[Array[String]] {
--- End diff --
By the way I just noticed, it looks like we're changing the public API
here. It used to return Array[String], but now it returns an Option. A second
point is that using `Try` here changes the semantics a little bit. Before we
propagate any exception that's not `NoSuchField*`, but now we swallow
everything.
I think we should revert this method to what it was before, and have the
caller deal with the fact that this can potentially be null.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
