From 0cc599708d213c6aeaf8ad1a748323980444eb15 Mon Sep 17 00:00:00 2001 From: uncleGen Date: Tue, 7 Feb 2017 11:26:56 +0800 Subject: [PATCH 1/4] Fail it if 'spark.master' is set with different value --- .../scala/org/apache/spark/SparkConf.scala | 26 +++++++++++++++++++ .../org/apache/spark/SparkConfSuite.scala | 20 ++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/core/src/main/scala/org/apache/spark/SparkConf.scala b/core/src/main/scala/org/apache/spark/SparkConf.scala index fe912e639bcbc..0c1401517f711 100644 --- a/core/src/main/scala/org/apache/spark/SparkConf.scala +++ b/core/src/main/scala/org/apache/spark/SparkConf.scala @@ -17,6 +17,7 @@ package org.apache.spark +import java.util.NoSuchElementException import java.util.concurrent.ConcurrentHashMap import scala.collection.JavaConverters._ @@ -91,6 +92,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria if (!silent) { logDeprecationWarning(key) } + checkValidSetting(key, value, this) settings.put(key, value) this } @@ -778,6 +780,30 @@ private[spark] object SparkConf extends Logging { } } + /** + * check if the given config key-value is valid. + */ + private def checkValidSetting(key: String, value: String, conf: SparkConf): Unit = { + key match { + case "spark.master" => + // First, there is no need to set 'spark.master' multi-times with different values. + // Second, It is possible for users to set the different 'spark.master' in code with + // `spark-submit` command, and will confuse users. + // So, we should do once check if the 'spark.master' already exists in settings and if + // the previous value is the same with current value. Throw a IllegalArgumentException when + // previous value is different with current value. + val previousOne = try { + Some(conf.get(key)) + } catch { + case e: NoSuchElementException => + None + } + if (previousOne.isDefined && !previousOne.get.equals(value)) { + throw new IllegalArgumentException("'spark.master' should not be ") + } + } + } + /** * Holds information about keys that have been deprecated and do not have a replacement. * diff --git a/core/src/test/scala/org/apache/spark/SparkConfSuite.scala b/core/src/test/scala/org/apache/spark/SparkConfSuite.scala index 0897891ee1758..f4416916105e2 100644 --- a/core/src/test/scala/org/apache/spark/SparkConfSuite.scala +++ b/core/src/test/scala/org/apache/spark/SparkConfSuite.scala @@ -322,6 +322,26 @@ class SparkConfSuite extends SparkFunSuite with LocalSparkContext with ResetSyst conf.validateSettings() } + test("set 'spark.master' with different value") { + val conf = new SparkConf() + conf.setMaster("local[4]") + try { + conf.setMaster("yarn-client") + assert(false, "previous: local[4], current: yarn-client") + } catch { + case e: IllegalArgumentException => + // expected + } + + try { + conf.set("spark.master", "yarn-cluster") + assert(false, "previous: local[4], current: yarn-cluster") + } catch { + case e: IllegalArgumentException => + // expected + } + } + } class Class1 {} From b50a0bd4b6df3aae7a8226e216ab46c1ea393c99 Mon Sep 17 00:00:00 2001 From: uncleGen Date: Tue, 7 Feb 2017 11:35:51 +0800 Subject: [PATCH 2/4] update --- core/src/main/scala/org/apache/spark/SparkConf.scala | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/src/main/scala/org/apache/spark/SparkConf.scala b/core/src/main/scala/org/apache/spark/SparkConf.scala index 0c1401517f711..79025a013f853 100644 --- a/core/src/main/scala/org/apache/spark/SparkConf.scala +++ b/core/src/main/scala/org/apache/spark/SparkConf.scala @@ -799,7 +799,8 @@ private[spark] object SparkConf extends Logging { None } if (previousOne.isDefined && !previousOne.get.equals(value)) { - throw new IllegalArgumentException("'spark.master' should not be ") + throw new IllegalArgumentException(s"'spark.master' should not be set with different " + + s"value, previous value is ${previousOne.get} and current value is $value") } } } From 8198db36574a1eb122c2d2303058b9424006f62e Mon Sep 17 00:00:00 2001 From: uncleGen Date: Tue, 7 Feb 2017 12:17:58 +0800 Subject: [PATCH 3/4] bug fix --- .../scala/org/apache/spark/SparkConf.scala | 37 +++++++++---------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/core/src/main/scala/org/apache/spark/SparkConf.scala b/core/src/main/scala/org/apache/spark/SparkConf.scala index 79025a013f853..5690aeeb52d11 100644 --- a/core/src/main/scala/org/apache/spark/SparkConf.scala +++ b/core/src/main/scala/org/apache/spark/SparkConf.scala @@ -783,25 +783,24 @@ private[spark] object SparkConf extends Logging { /** * check if the given config key-value is valid. */ - private def checkValidSetting(key: String, value: String, conf: SparkConf): Unit = { - key match { - case "spark.master" => - // First, there is no need to set 'spark.master' multi-times with different values. - // Second, It is possible for users to set the different 'spark.master' in code with - // `spark-submit` command, and will confuse users. - // So, we should do once check if the 'spark.master' already exists in settings and if - // the previous value is the same with current value. Throw a IllegalArgumentException when - // previous value is different with current value. - val previousOne = try { - Some(conf.get(key)) - } catch { - case e: NoSuchElementException => - None - } - if (previousOne.isDefined && !previousOne.get.equals(value)) { - throw new IllegalArgumentException(s"'spark.master' should not be set with different " + - s"value, previous value is ${previousOne.get} and current value is $value") - } + def checkValidSetting(key: String, value: String, conf: SparkConf): Unit = { + if (key.equals("spark.master")) { + // First, there is no need to set 'spark.master' multi-times with different values. + // Second, It is possible for users to set the different 'spark.master' in code with + // `spark-submit` command, and will confuse users. + // So, we should do once check if the 'spark.master' already exists in settings and if + // the previous value is the same with current value. Throw a IllegalArgumentException when + // previous value is different with current value. + val previousOne = try { + Some(conf.get(key)) + } catch { + case e: NoSuchElementException => + None + } + if (previousOne.isDefined && !previousOne.get.equals(value)) { + throw new IllegalArgumentException(s"'spark.master' should not be set with different " + + s"value, previous value is ${previousOne.get} and current value is $value") + } } } From 8565f9750990b466b4b26f086b71e9e1720fed4d Mon Sep 17 00:00:00 2001 From: uncleGen Date: Tue, 7 Feb 2017 17:13:36 +0800 Subject: [PATCH 4/4] fix unit test failure --- core/src/main/scala/org/apache/spark/SparkConf.scala | 2 +- core/src/test/scala/org/apache/spark/SparkConfSuite.scala | 3 +-- .../spark/deploy/rest/StandaloneRestSubmitSuite.scala | 1 - core/src/test/scala/org/apache/spark/util/UtilsSuite.scala | 1 + .../src/test/scala/org/apache/spark/repl/ReplSuite.scala | 6 +++++- .../sql/execution/streaming/state/StateStoreRDDSuite.scala | 4 +++- .../org/apache/spark/sql/hive/HiveSparkSubmitSuite.scala | 1 + .../main/scala/org/apache/spark/streaming/Checkpoint.scala | 2 ++ 8 files changed, 14 insertions(+), 6 deletions(-) diff --git a/core/src/main/scala/org/apache/spark/SparkConf.scala b/core/src/main/scala/org/apache/spark/SparkConf.scala index 5690aeeb52d11..812cfe142c2eb 100644 --- a/core/src/main/scala/org/apache/spark/SparkConf.scala +++ b/core/src/main/scala/org/apache/spark/SparkConf.scala @@ -799,7 +799,7 @@ private[spark] object SparkConf extends Logging { } if (previousOne.isDefined && !previousOne.get.equals(value)) { throw new IllegalArgumentException(s"'spark.master' should not be set with different " + - s"value, previous value is ${previousOne.get} and current value is $value") + s"values, previous value is ${previousOne.get} and current value is $value.") } } } diff --git a/core/src/test/scala/org/apache/spark/SparkConfSuite.scala b/core/src/test/scala/org/apache/spark/SparkConfSuite.scala index f4416916105e2..92a2ed65fa5cd 100644 --- a/core/src/test/scala/org/apache/spark/SparkConfSuite.scala +++ b/core/src/test/scala/org/apache/spark/SparkConfSuite.scala @@ -131,9 +131,8 @@ class SparkConfSuite extends SparkFunSuite with LocalSparkContext with ResetSyst } test("SparkContext property overriding") { - val conf = new SparkConf(false).setMaster("local").setAppName("My app") + val conf = new SparkConf(false).setAppName("My app") sc = new SparkContext("local[2]", "My other app", conf) - assert(sc.master === "local[2]") assert(sc.appName === "My other app") } diff --git a/core/src/test/scala/org/apache/spark/deploy/rest/StandaloneRestSubmitSuite.scala b/core/src/test/scala/org/apache/spark/deploy/rest/StandaloneRestSubmitSuite.scala index dd50e33da30ac..5458dfcc0efbc 100644 --- a/core/src/test/scala/org/apache/spark/deploy/rest/StandaloneRestSubmitSuite.scala +++ b/core/src/test/scala/org/apache/spark/deploy/rest/StandaloneRestSubmitSuite.scala @@ -440,7 +440,6 @@ class StandaloneRestSubmitSuite extends SparkFunSuite with BeforeAndAfterEach { val mainJar = "dummy-jar-not-used.jar" val commandLineArgs = Array( "--deploy-mode", "cluster", - "--master", masterUrl, "--name", mainClass, "--class", mainClass, mainJar) ++ appArgs diff --git a/core/src/test/scala/org/apache/spark/util/UtilsSuite.scala b/core/src/test/scala/org/apache/spark/util/UtilsSuite.scala index 6027310a963e5..e638149e452d0 100644 --- a/core/src/test/scala/org/apache/spark/util/UtilsSuite.scala +++ b/core/src/test/scala/org/apache/spark/util/UtilsSuite.scala @@ -845,6 +845,7 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging { conf.set("spark.executor.instances", "1")) === true) assert(Utils.isDynamicAllocationEnabled( conf.set("spark.executor.instances", "0")) === true) + conf.remove("spark.master") assert(Utils.isDynamicAllocationEnabled(conf.set("spark.master", "local")) === false) assert(Utils.isDynamicAllocationEnabled(conf.set("spark.dynamicAllocation.testing", "true"))) } diff --git a/repl/scala-2.11/src/test/scala/org/apache/spark/repl/ReplSuite.scala b/repl/scala-2.11/src/test/scala/org/apache/spark/repl/ReplSuite.scala index 55c91675ed3ba..cd4c9a2cde4f5 100644 --- a/repl/scala-2.11/src/test/scala/org/apache/spark/repl/ReplSuite.scala +++ b/repl/scala-2.11/src/test/scala/org/apache/spark/repl/ReplSuite.scala @@ -50,7 +50,11 @@ class ReplSuite extends SparkFunSuite { val oldExecutorClasspath = System.getProperty(CONF_EXECUTOR_CLASSPATH) System.setProperty(CONF_EXECUTOR_CLASSPATH, classpath) Main.sparkContext = null - Main.sparkSession = null // causes recreation of SparkContext for each test. + // causes recreation of SparkContext for each test. + Main.sparkSession = null + // (trick) avoid SPARK-19482. + // remove the previous 'spark.master' before set new one for each test. + Main.conf.remove("spark.master") Main.conf.set("spark.master", master) Main.doMain(Array("-classpath", classpath), new SparkILoop(in, new PrintWriter(out))) diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/StateStoreRDDSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/StateStoreRDDSuite.scala index bd197be655d58..4451316b9bdb2 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/StateStoreRDDSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/StateStoreRDDSuite.scala @@ -181,7 +181,9 @@ class StateStoreRDDSuite extends SparkFunSuite with BeforeAndAfter with BeforeAn withSparkSession( SparkSession.builder - .config(sparkConf.setMaster("local-cluster[2, 1, 1024]")) + // (trick) avoid SPARK-19482. + // remove the previous 'spark.master' before set new one for each test. + .config(sparkConf.remove("spark.master").setMaster("local-cluster[2, 1, 1024]")) .getOrCreate()) { spark => implicit val sqlContext = spark.sqlContext val path = Utils.createDirectory(tempDir, Random.nextString(10)).toString diff --git a/sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveSparkSubmitSuite.scala b/sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveSparkSubmitSuite.scala index 8f0d5d886c9d5..240ab15ccbb97 100644 --- a/sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveSparkSubmitSuite.scala +++ b/sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveSparkSubmitSuite.scala @@ -157,6 +157,7 @@ class HiveSparkSubmitSuite val jarDir = getTestResourcePath("regression-test-SPARK-8489") val testJar = s"$jarDir/test-$version.jar" val args = Seq( + "--master", "local", "--conf", "spark.ui.enabled=false", "--conf", "spark.master.rest.enabled=false", "--driver-java-options", "-Dderby.system.durability=test", diff --git a/streaming/src/main/scala/org/apache/spark/streaming/Checkpoint.scala b/streaming/src/main/scala/org/apache/spark/streaming/Checkpoint.scala index 5cbad8bf3ce6e..675a5cfb47aa1 100644 --- a/streaming/src/main/scala/org/apache/spark/streaming/Checkpoint.scala +++ b/streaming/src/main/scala/org/apache/spark/streaming/Checkpoint.scala @@ -63,6 +63,8 @@ class Checkpoint(ssc: StreamingContext, val checkpointTime: Time) val newReloadConf = new SparkConf(loadDefaults = true) propertiesToReload.foreach { prop => newReloadConf.getOption(prop).foreach { value => + // (trick) avoid SPARK-19482. + newSparkConf.remove(prop) newSparkConf.set(prop, value) } }