From 94b4dfaadc62182df45185e4c2d7bc7da6c1ee45 Mon Sep 17 00:00:00 2001 From: Andrew Or Date: Thu, 26 Feb 2015 16:57:20 -0800 Subject: [PATCH 1/5] Translate on get, not set --- core/src/main/scala/org/apache/spark/SparkConf.scala | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/core/src/main/scala/org/apache/spark/SparkConf.scala b/core/src/main/scala/org/apache/spark/SparkConf.scala index 0f4922ab4e31..5272031bc4a8 100644 --- a/core/src/main/scala/org/apache/spark/SparkConf.scala +++ b/core/src/main/scala/org/apache/spark/SparkConf.scala @@ -68,7 +68,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging { if (value == null) { throw new NullPointerException("null value for " + key) } - settings.put(translateConfKey(key, warn = true), value) + settings.put(key, value) this } @@ -140,7 +140,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging { /** Set a parameter if it isn't already configured */ def setIfMissing(key: String, value: String): SparkConf = { - settings.putIfAbsent(translateConfKey(key, warn = true), value) + settings.putIfAbsent(key, value) this } @@ -176,7 +176,8 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging { /** Get a parameter as an Option */ def getOption(key: String): Option[String] = { - Option(settings.get(translateConfKey(key))) + val warnIfDeprecated = contains(key) + Option(settings.get(translateConfKey(key, warnIfDeprecated))) } /** Get all parameters as a list of pairs */ @@ -229,7 +230,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging { def getAppId: String = get("spark.app.id") /** Does the configuration contain a given parameter? */ - def contains(key: String): Boolean = settings.containsKey(translateConfKey(key)) + def contains(key: String): Boolean = settings.containsKey(key) /** Copy this object */ override def clone: SparkConf = { From fef6c9c3e5b185933c2ef6e1fa01b130bda10438 Mon Sep 17 00:00:00 2001 From: Andrew Or Date: Thu, 26 Feb 2015 18:32:10 -0800 Subject: [PATCH 2/5] Restore deprecation logic for spark.executor.userClassPathFirst --- .../org/apache/spark/executor/Executor.scala | 12 ++++++---- .../org/apache/spark/SparkConfSuite.scala | 22 +++++++++++++------ .../org/apache/spark/deploy/yarn/Client.scala | 3 ++- 3 files changed, 25 insertions(+), 12 deletions(-) diff --git a/core/src/main/scala/org/apache/spark/executor/Executor.scala b/core/src/main/scala/org/apache/spark/executor/Executor.scala index b684fb704956..3091ac769853 100644 --- a/core/src/main/scala/org/apache/spark/executor/Executor.scala +++ b/core/src/main/scala/org/apache/spark/executor/Executor.scala @@ -92,6 +92,11 @@ private[spark] class Executor( private val executorActor = env.actorSystem.actorOf( Props(new ExecutorActor(executorId)), "ExecutorActor") + // Whether to load classes in user jars before those in Spark jars + private val userClassPathFirst = + conf.getBoolean("spark.executor.userClassPathFirst", + conf.getBoolean("spark.files.userClassPathFirst", false)) + // Create our ClassLoader // do this after SparkEnv creation so can access the SecurityManager private val urlClassLoader = createClassLoader() @@ -309,7 +314,7 @@ private[spark] class Executor( val urls = userClassPath.toArray ++ currentJars.keySet.map { uri => new File(uri.split("/").last).toURI.toURL } - if (conf.getBoolean("spark.executor.userClassPathFirst", false)) { + if (userClassPathFirst) { new ChildFirstURLClassLoader(urls, currentLoader) } else { new MutableURLClassLoader(urls, currentLoader) @@ -324,14 +329,13 @@ private[spark] class Executor( val classUri = conf.get("spark.repl.class.uri", null) if (classUri != null) { logInfo("Using REPL class URI: " + classUri) - val userClassPathFirst: java.lang.Boolean = - conf.getBoolean("spark.executor.userClassPathFirst", false) try { + val _userClassPathFirst: java.lang.Boolean = userClassPathFirst val klass = Class.forName("org.apache.spark.repl.ExecutorClassLoader") .asInstanceOf[Class[_ <: ClassLoader]] val constructor = klass.getConstructor(classOf[SparkConf], classOf[String], classOf[ClassLoader], classOf[Boolean]) - constructor.newInstance(conf, classUri, parent, userClassPathFirst) + constructor.newInstance(conf, classUri, parent, _userClassPathFirst) } catch { case _: ClassNotFoundException => logError("Could not find org.apache.spark.repl.ExecutorClassLoader on classpath!") diff --git a/core/src/test/scala/org/apache/spark/SparkConfSuite.scala b/core/src/test/scala/org/apache/spark/SparkConfSuite.scala index ea6b73bc68b3..6abe9f8cf922 100644 --- a/core/src/test/scala/org/apache/spark/SparkConfSuite.scala +++ b/core/src/test/scala/org/apache/spark/SparkConfSuite.scala @@ -198,15 +198,23 @@ class SparkConfSuite extends FunSuite with LocalSparkContext with ResetSystemPro } test("deprecated config keys") { + val conf = new SparkConf().set("spark.executor.userClassPathFirst", "true") + assert(conf.contains("spark.executor.userClassPathFirst")) + assert(!conf.contains("spark.files.userClassPathFirst")) + assert(conf.get("spark.executor.userClassPathFirst") === "true") + assert(conf.get("spark.files.userClassPathFirst") === "true") + } + + test("deprecated config does not override original config") { val conf = new SparkConf() .set("spark.files.userClassPathFirst", "true") - .set("spark.yarn.user.classpath.first", "true") - assert(conf.contains("spark.files.userClassPathFirst")) - assert(conf.contains("spark.executor.userClassPathFirst")) - assert(conf.contains("spark.yarn.user.classpath.first")) - assert(conf.getBoolean("spark.files.userClassPathFirst", false)) - assert(conf.getBoolean("spark.executor.userClassPathFirst", false)) - assert(conf.getBoolean("spark.yarn.user.classpath.first", false)) + .set("spark.executor.userClassPathFirst", "false") + val conf2 = new SparkConf() + .set("spark.executor.userClassPathFirst", "false") + .set("spark.files.userClassPathFirst", "true") + // In both cases, the new config's value should be used + assert(conf.get("spark.executor.userClassPathFirst") === "false") + assert(conf2.get("spark.executor.userClassPathFirst") === "false") } } diff --git a/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala b/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala index 46d9df93488c..61f8fc3f5a01 100644 --- a/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala +++ b/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala @@ -955,7 +955,8 @@ object Client extends Logging { if (isDriver) { conf.getBoolean("spark.driver.userClassPathFirst", false) } else { - conf.getBoolean("spark.executor.userClassPathFirst", false) + conf.getBoolean("spark.executor.userClassPathFirst", + conf.getBoolean("spark.files.userClassPathFirst", false)) } } From c26a9e3c3f6a17ae01782278fb1d4a1426fcbdbd Mon Sep 17 00:00:00 2001 From: Andrew Or Date: Fri, 27 Feb 2015 18:03:44 -0800 Subject: [PATCH 3/5] Revert all translate logic in SparkConf --- .../scala/org/apache/spark/SparkConf.scala | 3 +-- .../org/apache/spark/executor/Executor.scala | 11 +++++++--- .../org/apache/spark/SparkConfSuite.scala | 20 ------------------- .../org/apache/spark/deploy/yarn/Client.scala | 8 ++++++-- 4 files changed, 15 insertions(+), 27 deletions(-) diff --git a/core/src/main/scala/org/apache/spark/SparkConf.scala b/core/src/main/scala/org/apache/spark/SparkConf.scala index 5272031bc4a8..6f379814106e 100644 --- a/core/src/main/scala/org/apache/spark/SparkConf.scala +++ b/core/src/main/scala/org/apache/spark/SparkConf.scala @@ -176,8 +176,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging { /** Get a parameter as an Option */ def getOption(key: String): Option[String] = { - val warnIfDeprecated = contains(key) - Option(settings.get(translateConfKey(key, warnIfDeprecated))) + Option(settings.get(key)) } /** Get all parameters as a list of pairs */ diff --git a/core/src/main/scala/org/apache/spark/executor/Executor.scala b/core/src/main/scala/org/apache/spark/executor/Executor.scala index 3091ac769853..d18d68d3fb6a 100644 --- a/core/src/main/scala/org/apache/spark/executor/Executor.scala +++ b/core/src/main/scala/org/apache/spark/executor/Executor.scala @@ -93,9 +93,14 @@ private[spark] class Executor( Props(new ExecutorActor(executorId)), "ExecutorActor") // Whether to load classes in user jars before those in Spark jars - private val userClassPathFirst = - conf.getBoolean("spark.executor.userClassPathFirst", - conf.getBoolean("spark.files.userClassPathFirst", false)) + private val userClassPathFirst: Boolean = { + val oldKey = "spark.files.userClassPathFirst" + val newKey = "spark.executor.userClassPathFirst" + if (conf.contains(oldKey)) { + logWarning(s"$oldKey is deprecated. Please use $newKey instead.") + } + conf.getBoolean(newKey, conf.getBoolean(oldKey, false)) + } // Create our ClassLoader // do this after SparkEnv creation so can access the SecurityManager diff --git a/core/src/test/scala/org/apache/spark/SparkConfSuite.scala b/core/src/test/scala/org/apache/spark/SparkConfSuite.scala index 6abe9f8cf922..e08210ae60d1 100644 --- a/core/src/test/scala/org/apache/spark/SparkConfSuite.scala +++ b/core/src/test/scala/org/apache/spark/SparkConfSuite.scala @@ -197,26 +197,6 @@ class SparkConfSuite extends FunSuite with LocalSparkContext with ResetSystemPro serializer.newInstance().serialize(new StringBuffer()) } - test("deprecated config keys") { - val conf = new SparkConf().set("spark.executor.userClassPathFirst", "true") - assert(conf.contains("spark.executor.userClassPathFirst")) - assert(!conf.contains("spark.files.userClassPathFirst")) - assert(conf.get("spark.executor.userClassPathFirst") === "true") - assert(conf.get("spark.files.userClassPathFirst") === "true") - } - - test("deprecated config does not override original config") { - val conf = new SparkConf() - .set("spark.files.userClassPathFirst", "true") - .set("spark.executor.userClassPathFirst", "false") - val conf2 = new SparkConf() - .set("spark.executor.userClassPathFirst", "false") - .set("spark.files.userClassPathFirst", "true") - // In both cases, the new config's value should be used - assert(conf.get("spark.executor.userClassPathFirst") === "false") - assert(conf2.get("spark.executor.userClassPathFirst") === "false") - } - } class Class1 {} diff --git a/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala b/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala index 61f8fc3f5a01..462a97dfcc0b 100644 --- a/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala +++ b/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala @@ -955,8 +955,12 @@ object Client extends Logging { if (isDriver) { conf.getBoolean("spark.driver.userClassPathFirst", false) } else { - conf.getBoolean("spark.executor.userClassPathFirst", - conf.getBoolean("spark.files.userClassPathFirst", false)) + val oldKey = "spark.files.userClassPathFirst" + val newKey = "spark.executor.userClassPathFirst" + if (conf.contains(oldKey)) { + logWarning(s"$oldKey is deprecated. Please use $newKey instead.") + } + conf.getBoolean(newKey, conf.getBoolean(oldKey, false)) } } From 10e77b5068f916270b3d2f344f077475207f7526 Mon Sep 17 00:00:00 2001 From: Andrew Or Date: Sat, 28 Feb 2015 23:08:01 -0800 Subject: [PATCH 4/5] Add documentation for deprecation precedence --- docs/configuration.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/configuration.md b/docs/configuration.md index c11787b17eb8..ae90fe1f8f6b 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -70,7 +70,9 @@ each line consists of a key and a value separated by whitespace. For example: Any values specified as flags or in the properties file will be passed on to the application and merged with those specified through SparkConf. Properties set directly on the SparkConf take highest precedence, then flags passed to `spark-submit` or `spark-shell`, then options -in the `spark-defaults.conf` file. +in the `spark-defaults.conf` file. A few configuration keys have been renamed since earlier +versions of Spark; in such cases, the older key names are still accepted, but take lower +precedence than any instance of the newer key. ## Viewing Spark Properties From 11c525b60e000fee18122da55f3a3c8d275ec52a Mon Sep 17 00:00:00 2001 From: Andrew Or Date: Mon, 2 Mar 2015 10:43:48 -0800 Subject: [PATCH 5/5] Move warning to driver --- core/src/main/scala/org/apache/spark/SparkConf.scala | 7 +++++++ .../main/scala/org/apache/spark/executor/Executor.scala | 8 ++------ .../main/scala/org/apache/spark/deploy/yarn/Client.scala | 8 ++------ 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/core/src/main/scala/org/apache/spark/SparkConf.scala b/core/src/main/scala/org/apache/spark/SparkConf.scala index c2511875a569..2ca19f53d2f0 100644 --- a/core/src/main/scala/org/apache/spark/SparkConf.scala +++ b/core/src/main/scala/org/apache/spark/SparkConf.scala @@ -343,6 +343,13 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging { } } } + + // Warn against the use of deprecated configs + deprecatedConfigs.values.foreach { dc => + if (contains(dc.oldName)) { + dc.warn() + } + } } /** diff --git a/core/src/main/scala/org/apache/spark/executor/Executor.scala b/core/src/main/scala/org/apache/spark/executor/Executor.scala index d18d68d3fb6a..bed0a08d4d51 100644 --- a/core/src/main/scala/org/apache/spark/executor/Executor.scala +++ b/core/src/main/scala/org/apache/spark/executor/Executor.scala @@ -94,12 +94,8 @@ private[spark] class Executor( // Whether to load classes in user jars before those in Spark jars private val userClassPathFirst: Boolean = { - val oldKey = "spark.files.userClassPathFirst" - val newKey = "spark.executor.userClassPathFirst" - if (conf.contains(oldKey)) { - logWarning(s"$oldKey is deprecated. Please use $newKey instead.") - } - conf.getBoolean(newKey, conf.getBoolean(oldKey, false)) + conf.getBoolean("spark.executor.userClassPathFirst", + conf.getBoolean("spark.files.userClassPathFirst", false)) } // Create our ClassLoader diff --git a/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala b/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala index 462a97dfcc0b..61f8fc3f5a01 100644 --- a/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala +++ b/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala @@ -955,12 +955,8 @@ object Client extends Logging { if (isDriver) { conf.getBoolean("spark.driver.userClassPathFirst", false) } else { - val oldKey = "spark.files.userClassPathFirst" - val newKey = "spark.executor.userClassPathFirst" - if (conf.contains(oldKey)) { - logWarning(s"$oldKey is deprecated. Please use $newKey instead.") - } - conf.getBoolean(newKey, conf.getBoolean(oldKey, false)) + conf.getBoolean("spark.executor.userClassPathFirst", + conf.getBoolean("spark.files.userClassPathFirst", false)) } }