Github user berngp commented on a diff in the pull request:
https://github.com/apache/spark/pull/433#discussion_r11787690
--- 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]] {
+ val field =
classOf[YarnConfiguration].getField("DEFAULT_YARN_APPLICATION_CLASSPATH")
+ field.get(null).asInstanceOf[Array[String]]
+ }.toOption
/**
* In Hadoop 0.23, the MR application classpath comes with the YARN
application
* classpath. In Hadoop 2.0, it's an array of Strings, and in 2.2+ it's
a String.
* So we need to use reflection to retrieve it.
*/
- def getDefaultMRApplicationClasspath(): Array[String] = {
- try {
- val field =
classOf[MRJobConfig].getField("DEFAULT_MAPREDUCE_APPLICATION_CLASSPATH")
- if (field.getType == classOf[String]) {
- StringUtils.getStrings(field.get(null).asInstanceOf[String])
- } else {
- field.get(null).asInstanceOf[Array[String]]
- }
- } catch {
- case err: NoSuchFieldError => null
- case err: NoSuchFieldException => null
+ def getDefaultMRApplicationClasspath: Option[Array[String]] =
Try[Array[String]] {
+ val field =
classOf[MRJobConfig].getField("DEFAULT_MAPREDUCE_APPLICATION_CLASSPATH")
+ if (field.getType == classOf[String]) {
+ StringUtils.getStrings(field.get(null).asInstanceOf[String]).toArray
+ } else {
+ field.get(null).asInstanceOf[Array[String]]
}
- }
+ }.toOption
--- End diff --
I will still Try to defend the argument that `Try` is a better option than
just returning `null` or throwing an exception that we assume the author
intended, which is not documented.
The `Try` monad itself defines that:
"only non-fatal exceptions are caught by the combinators on Try (see
scala.util.control.NonFatal). Serious system errors, on the other hand, will be
thrown."
Assuming the intentions of the original author was to handle the `null` and
_bubble_ up any other exception that wasn't caught by the `try-catch` control
flow he was actually failing on the first one. To be honest I will bet that
most likely the `try-catch` specifically was used to handle the compilation
error flags due the usage of reflection. I know I am changing the _public_ API
but do think that is for the best.
Ref:
(NonFatal)[http://www.scala-lang.org/api/2.10.3/index.html#scala.util.control.NonFatal$]
(Try)[http://www.scala-lang.org/api/2.10.3/index.html#scala.util.Try]
---
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.
---
