From f198f28b1a3d7380a09e5687438a264101cc6965 Mon Sep 17 00:00:00 2001
From: William Sheu <william.sheu@databricks.com>
Date: Tue, 29 May 2018 11:35:25 -0700
Subject: [PATCH 1/6] [SPARK-24337][Core] Improve error messages for Spark conf
 values

---
 .../scala/org/apache/spark/SparkConf.scala    | 81 ++++++++++++++-----
 .../org/apache/spark/SparkConfSuite.scala     | 36 ++++++++-
 2 files changed, 95 insertions(+), 22 deletions(-)

diff --git a/core/src/main/scala/org/apache/spark/SparkConf.scala b/core/src/main/scala/org/apache/spark/SparkConf.scala
index dab409572646f..2c549f36b2ec0 100644
--- a/core/src/main/scala/org/apache/spark/SparkConf.scala
+++ b/core/src/main/scala/org/apache/spark/SparkConf.scala
@@ -265,16 +265,18 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * Get a time parameter as seconds; throws a NoSuchElementException if it's not set. If no
    * suffix is provided then seconds are assumed.
    * @throws java.util.NoSuchElementException If the time parameter is not set
+   * @throws IllegalArgumentException If the value can't be interpreted as seconds
    */
-  def getTimeAsSeconds(key: String): Long = {
+  def getTimeAsSeconds(key: String): Long = catchIllegalArgument(key) {
     Utils.timeStringAsSeconds(get(key))
   }
 
   /**
    * Get a time parameter as seconds, falling back to a default if not set. If no
    * suffix is provided then seconds are assumed.
+   * @throws IllegalArgumentException If the value can't be interpreted as seconds
    */
-  def getTimeAsSeconds(key: String, defaultValue: String): Long = {
+  def getTimeAsSeconds(key: String, defaultValue: String): Long = catchIllegalArgument(key) {
     Utils.timeStringAsSeconds(get(key, defaultValue))
   }
 
@@ -282,16 +284,18 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * Get a time parameter as milliseconds; throws a NoSuchElementException if it's not set. If no
    * suffix is provided then milliseconds are assumed.
    * @throws java.util.NoSuchElementException If the time parameter is not set
+   * @throws IllegalArgumentException If the value can't be interpreted as milliseconds
    */
-  def getTimeAsMs(key: String): Long = {
+  def getTimeAsMs(key: String): Long = catchIllegalArgument(key) {
     Utils.timeStringAsMs(get(key))
   }
 
   /**
    * Get a time parameter as milliseconds, falling back to a default if not set. If no
    * suffix is provided then milliseconds are assumed.
+   * @throws IllegalArgumentException If the value can't be interpreted as milliseconds
    */
-  def getTimeAsMs(key: String, defaultValue: String): Long = {
+  def getTimeAsMs(key: String, defaultValue: String): Long = catchIllegalArgument(key) {
     Utils.timeStringAsMs(get(key, defaultValue))
   }
 
@@ -299,23 +303,26 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * Get a size parameter as bytes; throws a NoSuchElementException if it's not set. If no
    * suffix is provided then bytes are assumed.
    * @throws java.util.NoSuchElementException If the size parameter is not set
+   * @throws IllegalArgumentException If the value can't be interpreted as bytes
    */
-  def getSizeAsBytes(key: String): Long = {
+  def getSizeAsBytes(key: String): Long = catchIllegalArgument(key) {
     Utils.byteStringAsBytes(get(key))
   }
 
   /**
    * Get a size parameter as bytes, falling back to a default if not set. If no
    * suffix is provided then bytes are assumed.
+   * @throws IllegalArgumentException If the value can't be interpreted as bytes
    */
-  def getSizeAsBytes(key: String, defaultValue: String): Long = {
+  def getSizeAsBytes(key: String, defaultValue: String): Long = catchIllegalArgument(key) {
     Utils.byteStringAsBytes(get(key, defaultValue))
   }
 
   /**
    * Get a size parameter as bytes, falling back to a default if not set.
+   * @throws IllegalArgumentException If the value can't be interpreted as bytes
    */
-  def getSizeAsBytes(key: String, defaultValue: Long): Long = {
+  def getSizeAsBytes(key: String, defaultValue: Long): Long = catchIllegalArgument(key) {
     Utils.byteStringAsBytes(get(key, defaultValue + "B"))
   }
 
@@ -323,16 +330,18 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * Get a size parameter as Kibibytes; throws a NoSuchElementException if it's not set. If no
    * suffix is provided then Kibibytes are assumed.
    * @throws java.util.NoSuchElementException If the size parameter is not set
+   * @throws IllegalArgumentException If the value can't be interpreted as Kibibytes
    */
-  def getSizeAsKb(key: String): Long = {
+  def getSizeAsKb(key: String): Long = catchIllegalArgument(key) {
     Utils.byteStringAsKb(get(key))
   }
 
   /**
    * Get a size parameter as Kibibytes, falling back to a default if not set. If no
    * suffix is provided then Kibibytes are assumed.
+   * @throws IllegalArgumentException If the value can't be interpreted as Kibibytes
    */
-  def getSizeAsKb(key: String, defaultValue: String): Long = {
+  def getSizeAsKb(key: String, defaultValue: String): Long = catchIllegalArgument(key) {
     Utils.byteStringAsKb(get(key, defaultValue))
   }
 
@@ -340,16 +349,18 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * Get a size parameter as Mebibytes; throws a NoSuchElementException if it's not set. If no
    * suffix is provided then Mebibytes are assumed.
    * @throws java.util.NoSuchElementException If the size parameter is not set
+   * @throws IllegalArgumentException If the value can't be interpreted as Mebibytes
    */
-  def getSizeAsMb(key: String): Long = {
+  def getSizeAsMb(key: String): Long = catchIllegalArgument(key) {
     Utils.byteStringAsMb(get(key))
   }
 
   /**
    * Get a size parameter as Mebibytes, falling back to a default if not set. If no
    * suffix is provided then Mebibytes are assumed.
+   * @throws IllegalArgumentException If the value can't be interpreted as Mebibytes
    */
-  def getSizeAsMb(key: String, defaultValue: String): Long = {
+  def getSizeAsMb(key: String, defaultValue: String): Long = catchIllegalArgument(key) {
     Utils.byteStringAsMb(get(key, defaultValue))
   }
 
@@ -357,16 +368,18 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * Get a size parameter as Gibibytes; throws a NoSuchElementException if it's not set. If no
    * suffix is provided then Gibibytes are assumed.
    * @throws java.util.NoSuchElementException If the size parameter is not set
+   * @throws IllegalArgumentException If the value can't be interpreted as Gibibytes
    */
-  def getSizeAsGb(key: String): Long = {
+  def getSizeAsGb(key: String): Long = catchIllegalArgument(key) {
     Utils.byteStringAsGb(get(key))
   }
 
   /**
    * Get a size parameter as Gibibytes, falling back to a default if not set. If no
    * suffix is provided then Gibibytes are assumed.
+   * @throws IllegalArgumentException If the value can't be interpreted as Gibibytes
    */
-  def getSizeAsGb(key: String, defaultValue: String): Long = {
+  def getSizeAsGb(key: String, defaultValue: String): Long = catchIllegalArgument(key) {
     Utils.byteStringAsGb(get(key, defaultValue))
   }
 
@@ -394,23 +407,35 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
   }
 
 
-  /** Get a parameter as an integer, falling back to a default if not set */
-  def getInt(key: String, defaultValue: Int): Int = {
+  /**
+   * Get a parameter as an integer, falling back to a default if not set
+   * @throws IllegalArgumentException If the value can't be interpreted as an integer
+   */
+  def getInt(key: String, defaultValue: Int): Int = catchIllegalArgument(key) {
     getOption(key).map(_.toInt).getOrElse(defaultValue)
   }
 
-  /** Get a parameter as a long, falling back to a default if not set */
-  def getLong(key: String, defaultValue: Long): Long = {
+  /**
+   * Get a parameter as a long, falling back to a default if not set
+   * @throws IllegalArgumentException If the value can't be interpreted as an long
+   */
+  def getLong(key: String, defaultValue: Long): Long = catchIllegalArgument(key) {
     getOption(key).map(_.toLong).getOrElse(defaultValue)
   }
 
-  /** Get a parameter as a double, falling back to a default if not set */
-  def getDouble(key: String, defaultValue: Double): Double = {
+  /**
+   * Get a parameter as a double, falling back to a default if not ste
+   * @throws IllegalArgumentException If the value can't be interpreted as an double
+   */
+  def getDouble(key: String, defaultValue: Double): Double = catchIllegalArgument(key) {
     getOption(key).map(_.toDouble).getOrElse(defaultValue)
   }
 
-  /** Get a parameter as a boolean, falling back to a default if not set */
-  def getBoolean(key: String, defaultValue: Boolean): Boolean = {
+  /**
+   * Get a parameter as a boolean, falling back to a default if not set
+   * @throws IllegalArgumentException If the value can't be interpreted as an boolean
+   */
+  def getBoolean(key: String, defaultValue: Boolean): Boolean = catchIllegalArgument(key) {
     getOption(key).map(_.toBoolean).getOrElse(defaultValue)
   }
 
@@ -448,6 +473,20 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    */
   private[spark] def getenv(name: String): String = System.getenv(name)
 
+  /**
+   * Wrapper method for get*() methods which require some specific value format. This catches
+   * any [[NumberFormatException]] or [[IllegalArgumentException]] and re-raises it with the
+   * incorrectly configured key in the exception message.
+   */
+  private def catchIllegalArgument[T](key: String)(getValue: => T): T = {
+    try {
+      getValue
+    } catch {
+      case e @ (_ : NumberFormatException | _ : IllegalArgumentException) =>
+        throw new IllegalArgumentException(s"Illegal value for config key $key", e)
+    }
+  }
+
   /**
    * Checks for illegal or deprecated config settings. Throws an exception for the former. Not
    * idempotent - may mutate this conf object to convert deprecated settings to supported ones.
diff --git a/core/src/test/scala/org/apache/spark/SparkConfSuite.scala b/core/src/test/scala/org/apache/spark/SparkConfSuite.scala
index bff808eb540ac..49937995bdefe 100644
--- a/core/src/test/scala/org/apache/spark/SparkConfSuite.scala
+++ b/core/src/test/scala/org/apache/spark/SparkConfSuite.scala
@@ -25,6 +25,7 @@ import scala.language.postfixOps
 import scala.util.{Random, Try}
 
 import com.esotericsoftware.kryo.Kryo
+import org.scalatest.Matchers
 
 import org.apache.spark.deploy.history.config._
 import org.apache.spark.internal.config._
@@ -32,7 +33,8 @@ import org.apache.spark.network.util.ByteUnit
 import org.apache.spark.serializer.{JavaSerializer, KryoRegistrator, KryoSerializer}
 import org.apache.spark.util.{ResetSystemProperties, RpcUtils}
 
-class SparkConfSuite extends SparkFunSuite with LocalSparkContext with ResetSystemProperties {
+class SparkConfSuite extends SparkFunSuite with LocalSparkContext with ResetSystemProperties with
+  Matchers {
   test("Test byteString conversion") {
     val conf = new SparkConf()
     // Simply exercise the API, we don't need a complete conversion test since that's handled in
@@ -339,6 +341,38 @@ class SparkConfSuite extends SparkFunSuite with LocalSparkContext with ResetSyst
     }
   }
 
+  val defaultIllegalValue = "SomeIllegalValue"
+  val illegalArgumentTests : Map[String, (SparkConf, String) => Any] = Map(
+    "getTimeAsSeconds" -> (_.getTimeAsSeconds(_)),
+    "getTimeAsSeconds with default" -> (_.getTimeAsSeconds(_, defaultIllegalValue)),
+    "getTimeAsMs" -> (_.getTimeAsMs(_)),
+    "getTimeAsMs with default" -> (_.getTimeAsMs(_, defaultIllegalValue)),
+    "getSizeAsBytes" -> (_.getSizeAsBytes(_)),
+    "getSizeAsBytes with default string" -> (_.getSizeAsBytes(_, defaultIllegalValue)),
+    "getSizeAsBytes with default long" -> (_.getSizeAsBytes(_, 0L)),
+    "getSizeAsKb" -> (_.getSizeAsKb(_)),
+    "getSizeAsKb with default" -> (_.getSizeAsKb(_, defaultIllegalValue)),
+    "getSizeAsMb" -> (_.getSizeAsMb(_)),
+    "getSizeAsMb with default" -> (_.getSizeAsMb(_, defaultIllegalValue)),
+    "getSizeAsGb" -> (_.getSizeAsGb(_)),
+    "getSizeAsGb with default" -> (_.getSizeAsGb(_, defaultIllegalValue)),
+    "getInt" -> (_.getInt(_, 0)),
+    "getLong" -> (_.getLong(_, 0L)),
+    "getDouble" -> (_.getDouble(_, 0.0)),
+    "getBoolean" -> (_.getBoolean(_, false))
+  )
+
+  illegalArgumentTests.foreach { case (name, getValue) =>
+    test(s"SPARK-24337: $name throws an useful error message with key name") {
+      val key = "SomeKey"
+      val conf = new SparkConf()
+      conf.set(key, "SomeInvalidValue")
+      val thrown = the [IllegalArgumentException] thrownBy {
+        getValue(conf, key)
+      }
+      thrown.getMessage should include (key)
+    }
+  }
 }
 
 class Class1 {}

From 586b0e70f2e1104e6a8fc0cbe9d390f0c4a4ac7e Mon Sep 17 00:00:00 2001
From: William Sheu <william.sheu@databricks.com>
Date: Tue, 29 May 2018 15:58:18 -0700
Subject: [PATCH 2/6] Address PR comments

 * Rename catchIllegalArgument -> catchIllegalValue
 * Fix minor grammatical errors
---
 .../scala/org/apache/spark/SparkConf.scala    | 42 +++++++++----------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/core/src/main/scala/org/apache/spark/SparkConf.scala b/core/src/main/scala/org/apache/spark/SparkConf.scala
index 2c549f36b2ec0..543d8262419d5 100644
--- a/core/src/main/scala/org/apache/spark/SparkConf.scala
+++ b/core/src/main/scala/org/apache/spark/SparkConf.scala
@@ -267,7 +267,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * @throws java.util.NoSuchElementException If the time parameter is not set
    * @throws IllegalArgumentException If the value can't be interpreted as seconds
    */
-  def getTimeAsSeconds(key: String): Long = catchIllegalArgument(key) {
+  def getTimeAsSeconds(key: String): Long = catchIllegalValue(key) {
     Utils.timeStringAsSeconds(get(key))
   }
 
@@ -276,7 +276,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * suffix is provided then seconds are assumed.
    * @throws IllegalArgumentException If the value can't be interpreted as seconds
    */
-  def getTimeAsSeconds(key: String, defaultValue: String): Long = catchIllegalArgument(key) {
+  def getTimeAsSeconds(key: String, defaultValue: String): Long = catchIllegalValue(key) {
     Utils.timeStringAsSeconds(get(key, defaultValue))
   }
 
@@ -286,7 +286,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * @throws java.util.NoSuchElementException If the time parameter is not set
    * @throws IllegalArgumentException If the value can't be interpreted as milliseconds
    */
-  def getTimeAsMs(key: String): Long = catchIllegalArgument(key) {
+  def getTimeAsMs(key: String): Long = catchIllegalValue(key) {
     Utils.timeStringAsMs(get(key))
   }
 
@@ -295,7 +295,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * suffix is provided then milliseconds are assumed.
    * @throws IllegalArgumentException If the value can't be interpreted as milliseconds
    */
-  def getTimeAsMs(key: String, defaultValue: String): Long = catchIllegalArgument(key) {
+  def getTimeAsMs(key: String, defaultValue: String): Long = catchIllegalValue(key) {
     Utils.timeStringAsMs(get(key, defaultValue))
   }
 
@@ -305,7 +305,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * @throws java.util.NoSuchElementException If the size parameter is not set
    * @throws IllegalArgumentException If the value can't be interpreted as bytes
    */
-  def getSizeAsBytes(key: String): Long = catchIllegalArgument(key) {
+  def getSizeAsBytes(key: String): Long = catchIllegalValue(key) {
     Utils.byteStringAsBytes(get(key))
   }
 
@@ -314,7 +314,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * suffix is provided then bytes are assumed.
    * @throws IllegalArgumentException If the value can't be interpreted as bytes
    */
-  def getSizeAsBytes(key: String, defaultValue: String): Long = catchIllegalArgument(key) {
+  def getSizeAsBytes(key: String, defaultValue: String): Long = catchIllegalValue(key) {
     Utils.byteStringAsBytes(get(key, defaultValue))
   }
 
@@ -322,7 +322,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * Get a size parameter as bytes, falling back to a default if not set.
    * @throws IllegalArgumentException If the value can't be interpreted as bytes
    */
-  def getSizeAsBytes(key: String, defaultValue: Long): Long = catchIllegalArgument(key) {
+  def getSizeAsBytes(key: String, defaultValue: Long): Long = catchIllegalValue(key) {
     Utils.byteStringAsBytes(get(key, defaultValue + "B"))
   }
 
@@ -332,7 +332,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * @throws java.util.NoSuchElementException If the size parameter is not set
    * @throws IllegalArgumentException If the value can't be interpreted as Kibibytes
    */
-  def getSizeAsKb(key: String): Long = catchIllegalArgument(key) {
+  def getSizeAsKb(key: String): Long = catchIllegalValue(key) {
     Utils.byteStringAsKb(get(key))
   }
 
@@ -341,7 +341,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * suffix is provided then Kibibytes are assumed.
    * @throws IllegalArgumentException If the value can't be interpreted as Kibibytes
    */
-  def getSizeAsKb(key: String, defaultValue: String): Long = catchIllegalArgument(key) {
+  def getSizeAsKb(key: String, defaultValue: String): Long = catchIllegalValue(key) {
     Utils.byteStringAsKb(get(key, defaultValue))
   }
 
@@ -351,7 +351,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * @throws java.util.NoSuchElementException If the size parameter is not set
    * @throws IllegalArgumentException If the value can't be interpreted as Mebibytes
    */
-  def getSizeAsMb(key: String): Long = catchIllegalArgument(key) {
+  def getSizeAsMb(key: String): Long = catchIllegalValue(key) {
     Utils.byteStringAsMb(get(key))
   }
 
@@ -360,7 +360,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * suffix is provided then Mebibytes are assumed.
    * @throws IllegalArgumentException If the value can't be interpreted as Mebibytes
    */
-  def getSizeAsMb(key: String, defaultValue: String): Long = catchIllegalArgument(key) {
+  def getSizeAsMb(key: String, defaultValue: String): Long = catchIllegalValue(key) {
     Utils.byteStringAsMb(get(key, defaultValue))
   }
 
@@ -370,7 +370,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * @throws java.util.NoSuchElementException If the size parameter is not set
    * @throws IllegalArgumentException If the value can't be interpreted as Gibibytes
    */
-  def getSizeAsGb(key: String): Long = catchIllegalArgument(key) {
+  def getSizeAsGb(key: String): Long = catchIllegalValue(key) {
     Utils.byteStringAsGb(get(key))
   }
 
@@ -379,7 +379,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * suffix is provided then Gibibytes are assumed.
    * @throws IllegalArgumentException If the value can't be interpreted as Gibibytes
    */
-  def getSizeAsGb(key: String, defaultValue: String): Long = catchIllegalArgument(key) {
+  def getSizeAsGb(key: String, defaultValue: String): Long = catchIllegalValue(key) {
     Utils.byteStringAsGb(get(key, defaultValue))
   }
 
@@ -411,31 +411,31 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * Get a parameter as an integer, falling back to a default if not set
    * @throws IllegalArgumentException If the value can't be interpreted as an integer
    */
-  def getInt(key: String, defaultValue: Int): Int = catchIllegalArgument(key) {
+  def getInt(key: String, defaultValue: Int): Int = catchIllegalValue(key) {
     getOption(key).map(_.toInt).getOrElse(defaultValue)
   }
 
   /**
    * Get a parameter as a long, falling back to a default if not set
-   * @throws IllegalArgumentException If the value can't be interpreted as an long
+   * @throws IllegalArgumentException If the value can't be interpreted as a long
    */
-  def getLong(key: String, defaultValue: Long): Long = catchIllegalArgument(key) {
+  def getLong(key: String, defaultValue: Long): Long = catchIllegalValue(key) {
     getOption(key).map(_.toLong).getOrElse(defaultValue)
   }
 
   /**
    * Get a parameter as a double, falling back to a default if not ste
-   * @throws IllegalArgumentException If the value can't be interpreted as an double
+   * @throws IllegalArgumentException If the value can't be interpreted as a double
    */
-  def getDouble(key: String, defaultValue: Double): Double = catchIllegalArgument(key) {
+  def getDouble(key: String, defaultValue: Double): Double = catchIllegalValue(key) {
     getOption(key).map(_.toDouble).getOrElse(defaultValue)
   }
 
   /**
    * Get a parameter as a boolean, falling back to a default if not set
-   * @throws IllegalArgumentException If the value can't be interpreted as an boolean
+   * @throws IllegalArgumentException If the value can't be interpreted as a boolean
    */
-  def getBoolean(key: String, defaultValue: Boolean): Boolean = catchIllegalArgument(key) {
+  def getBoolean(key: String, defaultValue: Boolean): Boolean = catchIllegalValue(key) {
     getOption(key).map(_.toBoolean).getOrElse(defaultValue)
   }
 
@@ -478,7 +478,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * any [[NumberFormatException]] or [[IllegalArgumentException]] and re-raises it with the
    * incorrectly configured key in the exception message.
    */
-  private def catchIllegalArgument[T](key: String)(getValue: => T): T = {
+  private def catchIllegalValue[T](key: String)(getValue: => T): T = {
     try {
       getValue
     } catch {

From badbf0e6766a99565e061063041f231d119d6d3a Mon Sep 17 00:00:00 2001
From: William Sheu <william.sheu@databricks.com>
Date: Tue, 29 May 2018 16:00:47 -0700
Subject: [PATCH 3/6] Update unit test variable names to match rename

---
 core/src/test/scala/org/apache/spark/SparkConfSuite.scala | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/core/src/test/scala/org/apache/spark/SparkConfSuite.scala b/core/src/test/scala/org/apache/spark/SparkConfSuite.scala
index 49937995bdefe..18cb813e2f635 100644
--- a/core/src/test/scala/org/apache/spark/SparkConfSuite.scala
+++ b/core/src/test/scala/org/apache/spark/SparkConfSuite.scala
@@ -342,7 +342,7 @@ class SparkConfSuite extends SparkFunSuite with LocalSparkContext with ResetSyst
   }
 
   val defaultIllegalValue = "SomeIllegalValue"
-  val illegalArgumentTests : Map[String, (SparkConf, String) => Any] = Map(
+  val illegalValueTests : Map[String, (SparkConf, String) => Any] = Map(
     "getTimeAsSeconds" -> (_.getTimeAsSeconds(_)),
     "getTimeAsSeconds with default" -> (_.getTimeAsSeconds(_, defaultIllegalValue)),
     "getTimeAsMs" -> (_.getTimeAsMs(_)),
@@ -362,7 +362,7 @@ class SparkConfSuite extends SparkFunSuite with LocalSparkContext with ResetSyst
     "getBoolean" -> (_.getBoolean(_, false))
   )
 
-  illegalArgumentTests.foreach { case (name, getValue) =>
+  illegalValueTests.foreach { case (name, getValue) =>
     test(s"SPARK-24337: $name throws an useful error message with key name") {
       val key = "SomeKey"
       val conf = new SparkConf()

From c1b4d1670fe1bb5c2b9b0d66c14e3da26627d29e Mon Sep 17 00:00:00 2001
From: William Sheu <william.sheu@databricks.com>
Date: Wed, 30 May 2018 10:17:51 -0700
Subject: [PATCH 4/6] Address PR comments

 * can't -> cannot
 * should include -> assert _.contains
 * Don't change exception type when re-raising
 * Don't use Scalatest matchers
---
 .../scala/org/apache/spark/SparkConf.scala    | 42 ++++++++++---------
 .../org/apache/spark/SparkConfSuite.scala     |  7 ++--
 2 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/core/src/main/scala/org/apache/spark/SparkConf.scala b/core/src/main/scala/org/apache/spark/SparkConf.scala
index 543d8262419d5..f2a28bb529634 100644
--- a/core/src/main/scala/org/apache/spark/SparkConf.scala
+++ b/core/src/main/scala/org/apache/spark/SparkConf.scala
@@ -265,7 +265,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * Get a time parameter as seconds; throws a NoSuchElementException if it's not set. If no
    * suffix is provided then seconds are assumed.
    * @throws java.util.NoSuchElementException If the time parameter is not set
-   * @throws IllegalArgumentException If the value can't be interpreted as seconds
+   * @throws NumberFormatException If the value cannot be interpreted as seconds
    */
   def getTimeAsSeconds(key: String): Long = catchIllegalValue(key) {
     Utils.timeStringAsSeconds(get(key))
@@ -274,7 +274,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
   /**
    * Get a time parameter as seconds, falling back to a default if not set. If no
    * suffix is provided then seconds are assumed.
-   * @throws IllegalArgumentException If the value can't be interpreted as seconds
+   * @throws NumberFormatException If the value cannot be interpreted as seconds
    */
   def getTimeAsSeconds(key: String, defaultValue: String): Long = catchIllegalValue(key) {
     Utils.timeStringAsSeconds(get(key, defaultValue))
@@ -284,7 +284,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * Get a time parameter as milliseconds; throws a NoSuchElementException if it's not set. If no
    * suffix is provided then milliseconds are assumed.
    * @throws java.util.NoSuchElementException If the time parameter is not set
-   * @throws IllegalArgumentException If the value can't be interpreted as milliseconds
+   * @throws NumberFormatException If the value cannot be interpreted as milliseconds
    */
   def getTimeAsMs(key: String): Long = catchIllegalValue(key) {
     Utils.timeStringAsMs(get(key))
@@ -293,7 +293,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
   /**
    * Get a time parameter as milliseconds, falling back to a default if not set. If no
    * suffix is provided then milliseconds are assumed.
-   * @throws IllegalArgumentException If the value can't be interpreted as milliseconds
+   * @throws NumberFormatException If the value cannot be interpreted as milliseconds
    */
   def getTimeAsMs(key: String, defaultValue: String): Long = catchIllegalValue(key) {
     Utils.timeStringAsMs(get(key, defaultValue))
@@ -303,7 +303,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * Get a size parameter as bytes; throws a NoSuchElementException if it's not set. If no
    * suffix is provided then bytes are assumed.
    * @throws java.util.NoSuchElementException If the size parameter is not set
-   * @throws IllegalArgumentException If the value can't be interpreted as bytes
+   * @throws NumberFormatException If the value cannot be interpreted as bytes
    */
   def getSizeAsBytes(key: String): Long = catchIllegalValue(key) {
     Utils.byteStringAsBytes(get(key))
@@ -312,7 +312,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
   /**
    * Get a size parameter as bytes, falling back to a default if not set. If no
    * suffix is provided then bytes are assumed.
-   * @throws IllegalArgumentException If the value can't be interpreted as bytes
+   * @throws NumberFormatException If the value cannot be interpreted as bytes
    */
   def getSizeAsBytes(key: String, defaultValue: String): Long = catchIllegalValue(key) {
     Utils.byteStringAsBytes(get(key, defaultValue))
@@ -320,7 +320,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
 
   /**
    * Get a size parameter as bytes, falling back to a default if not set.
-   * @throws IllegalArgumentException If the value can't be interpreted as bytes
+   * @throws NumberFormatException If the value cannot be interpreted as bytes
    */
   def getSizeAsBytes(key: String, defaultValue: Long): Long = catchIllegalValue(key) {
     Utils.byteStringAsBytes(get(key, defaultValue + "B"))
@@ -330,7 +330,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * Get a size parameter as Kibibytes; throws a NoSuchElementException if it's not set. If no
    * suffix is provided then Kibibytes are assumed.
    * @throws java.util.NoSuchElementException If the size parameter is not set
-   * @throws IllegalArgumentException If the value can't be interpreted as Kibibytes
+   * @throws NumberFormatException If the value cannot be interpreted as Kibibytes
    */
   def getSizeAsKb(key: String): Long = catchIllegalValue(key) {
     Utils.byteStringAsKb(get(key))
@@ -339,7 +339,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
   /**
    * Get a size parameter as Kibibytes, falling back to a default if not set. If no
    * suffix is provided then Kibibytes are assumed.
-   * @throws IllegalArgumentException If the value can't be interpreted as Kibibytes
+   * @throws NumberFormatException If the value cannot be interpreted as Kibibytes
    */
   def getSizeAsKb(key: String, defaultValue: String): Long = catchIllegalValue(key) {
     Utils.byteStringAsKb(get(key, defaultValue))
@@ -349,7 +349,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * Get a size parameter as Mebibytes; throws a NoSuchElementException if it's not set. If no
    * suffix is provided then Mebibytes are assumed.
    * @throws java.util.NoSuchElementException If the size parameter is not set
-   * @throws IllegalArgumentException If the value can't be interpreted as Mebibytes
+   * @throws NumberFormatException If the value cannot be interpreted as Mebibytes
    */
   def getSizeAsMb(key: String): Long = catchIllegalValue(key) {
     Utils.byteStringAsMb(get(key))
@@ -358,7 +358,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
   /**
    * Get a size parameter as Mebibytes, falling back to a default if not set. If no
    * suffix is provided then Mebibytes are assumed.
-   * @throws IllegalArgumentException If the value can't be interpreted as Mebibytes
+   * @throws NumberFormatException If the value cannot be interpreted as Mebibytes
    */
   def getSizeAsMb(key: String, defaultValue: String): Long = catchIllegalValue(key) {
     Utils.byteStringAsMb(get(key, defaultValue))
@@ -368,7 +368,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
    * Get a size parameter as Gibibytes; throws a NoSuchElementException if it's not set. If no
    * suffix is provided then Gibibytes are assumed.
    * @throws java.util.NoSuchElementException If the size parameter is not set
-   * @throws IllegalArgumentException If the value can't be interpreted as Gibibytes
+   * @throws NumberFormatException If the value cannot be interpreted as Gibibytes
    */
   def getSizeAsGb(key: String): Long = catchIllegalValue(key) {
     Utils.byteStringAsGb(get(key))
@@ -377,7 +377,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
   /**
    * Get a size parameter as Gibibytes, falling back to a default if not set. If no
    * suffix is provided then Gibibytes are assumed.
-   * @throws IllegalArgumentException If the value can't be interpreted as Gibibytes
+   * @throws NumberFormatException If the value cannot be interpreted as Gibibytes
    */
   def getSizeAsGb(key: String, defaultValue: String): Long = catchIllegalValue(key) {
     Utils.byteStringAsGb(get(key, defaultValue))
@@ -409,7 +409,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
 
   /**
    * Get a parameter as an integer, falling back to a default if not set
-   * @throws IllegalArgumentException If the value can't be interpreted as an integer
+   * @throws NumberFormatException If the value cannot be interpreted as an integer
    */
   def getInt(key: String, defaultValue: Int): Int = catchIllegalValue(key) {
     getOption(key).map(_.toInt).getOrElse(defaultValue)
@@ -417,7 +417,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
 
   /**
    * Get a parameter as a long, falling back to a default if not set
-   * @throws IllegalArgumentException If the value can't be interpreted as a long
+   * @throws NumberFormatException If the value cannot be interpreted as a long
    */
   def getLong(key: String, defaultValue: Long): Long = catchIllegalValue(key) {
     getOption(key).map(_.toLong).getOrElse(defaultValue)
@@ -425,7 +425,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
 
   /**
    * Get a parameter as a double, falling back to a default if not ste
-   * @throws IllegalArgumentException If the value can't be interpreted as a double
+   * @throws NumberFormatException If the value cannot be interpreted as a double
    */
   def getDouble(key: String, defaultValue: Double): Double = catchIllegalValue(key) {
     getOption(key).map(_.toDouble).getOrElse(defaultValue)
@@ -433,7 +433,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
 
   /**
    * Get a parameter as a boolean, falling back to a default if not set
-   * @throws IllegalArgumentException If the value can't be interpreted as a boolean
+   * @throws IllegalArgumentException If the value cannot be interpreted as a boolean
    */
   def getBoolean(key: String, defaultValue: Boolean): Boolean = catchIllegalValue(key) {
     getOption(key).map(_.toBoolean).getOrElse(defaultValue)
@@ -474,7 +474,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
   private[spark] def getenv(name: String): String = System.getenv(name)
 
   /**
-   * Wrapper method for get*() methods which require some specific value format. This catches
+   * Wrapper method for get() methods which require some specific value format. This catches
    * any [[NumberFormatException]] or [[IllegalArgumentException]] and re-raises it with the
    * incorrectly configured key in the exception message.
    */
@@ -482,8 +482,10 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
     try {
       getValue
     } catch {
-      case e @ (_ : NumberFormatException | _ : IllegalArgumentException) =>
-        throw new IllegalArgumentException(s"Illegal value for config key $key", e)
+      case e: NumberFormatException =>
+        throw new NumberFormatException(s"Illegal value for config key $key: ${e.getMessage}")
+      case e: IllegalArgumentException =>
+        throw new IllegalArgumentException(s"Illegal value for config key $key: ${e.getMessage}")
     }
   }
 
diff --git a/core/src/test/scala/org/apache/spark/SparkConfSuite.scala b/core/src/test/scala/org/apache/spark/SparkConfSuite.scala
index 18cb813e2f635..a023955ef939e 100644
--- a/core/src/test/scala/org/apache/spark/SparkConfSuite.scala
+++ b/core/src/test/scala/org/apache/spark/SparkConfSuite.scala
@@ -33,8 +33,7 @@ import org.apache.spark.network.util.ByteUnit
 import org.apache.spark.serializer.{JavaSerializer, KryoRegistrator, KryoSerializer}
 import org.apache.spark.util.{ResetSystemProperties, RpcUtils}
 
-class SparkConfSuite extends SparkFunSuite with LocalSparkContext with ResetSystemProperties with
-  Matchers {
+class SparkConfSuite extends SparkFunSuite with LocalSparkContext with ResetSystemProperties {
   test("Test byteString conversion") {
     val conf = new SparkConf()
     // Simply exercise the API, we don't need a complete conversion test since that's handled in
@@ -367,10 +366,10 @@ class SparkConfSuite extends SparkFunSuite with LocalSparkContext with ResetSyst
       val key = "SomeKey"
       val conf = new SparkConf()
       conf.set(key, "SomeInvalidValue")
-      val thrown = the [IllegalArgumentException] thrownBy {
+      val thrown = intercept [IllegalArgumentException] {
         getValue(conf, key)
       }
-      thrown.getMessage should include (key)
+      assert(thrown.getMessage.contains(key))
     }
   }
 }

From b7ff38f16c91b7df326f49d1f821b14a6dc82e8d Mon Sep 17 00:00:00 2001
From: William Sheu <william.sheu@databricks.com>
Date: Wed, 30 May 2018 16:35:21 -0700
Subject: [PATCH 5/6] Address more PR comments

 * Remove extra space in tests
 * Add cause to thrown exceptions
---
 core/src/main/scala/org/apache/spark/SparkConf.scala      | 4 +++-
 core/src/test/scala/org/apache/spark/SparkConfSuite.scala | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/core/src/main/scala/org/apache/spark/SparkConf.scala b/core/src/main/scala/org/apache/spark/SparkConf.scala
index f2a28bb529634..6c4c5c94cfa28 100644
--- a/core/src/main/scala/org/apache/spark/SparkConf.scala
+++ b/core/src/main/scala/org/apache/spark/SparkConf.scala
@@ -483,9 +483,11 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
       getValue
     } catch {
       case e: NumberFormatException =>
+        // NumberFormatException doesn't have a constructor that takes a cause for some reason.
         throw new NumberFormatException(s"Illegal value for config key $key: ${e.getMessage}")
+            .initCause(e)
       case e: IllegalArgumentException =>
-        throw new IllegalArgumentException(s"Illegal value for config key $key: ${e.getMessage}")
+        throw new IllegalArgumentException(s"Illegal value for config key $key: ${e.getMessage}", e)
     }
   }
 
diff --git a/core/src/test/scala/org/apache/spark/SparkConfSuite.scala b/core/src/test/scala/org/apache/spark/SparkConfSuite.scala
index a023955ef939e..a1bbc89ad8a0b 100644
--- a/core/src/test/scala/org/apache/spark/SparkConfSuite.scala
+++ b/core/src/test/scala/org/apache/spark/SparkConfSuite.scala
@@ -366,7 +366,7 @@ class SparkConfSuite extends SparkFunSuite with LocalSparkContext with ResetSyst
       val key = "SomeKey"
       val conf = new SparkConf()
       conf.set(key, "SomeInvalidValue")
-      val thrown = intercept [IllegalArgumentException] {
+      val thrown = intercept[IllegalArgumentException] {
         getValue(conf, key)
       }
       assert(thrown.getMessage.contains(key))

From 38ffa3e551c7eee69d12cc736d33d137abd333b7 Mon Sep 17 00:00:00 2001
From: William Sheu <william.sheu@databricks.com>
Date: Wed, 30 May 2018 16:57:24 -0700
Subject: [PATCH 6/6] Remove unnecessary import

---
 core/src/test/scala/org/apache/spark/SparkConfSuite.scala | 1 -
 1 file changed, 1 deletion(-)

diff --git a/core/src/test/scala/org/apache/spark/SparkConfSuite.scala b/core/src/test/scala/org/apache/spark/SparkConfSuite.scala
index a1bbc89ad8a0b..0d06b02e74e34 100644
--- a/core/src/test/scala/org/apache/spark/SparkConfSuite.scala
+++ b/core/src/test/scala/org/apache/spark/SparkConfSuite.scala
@@ -25,7 +25,6 @@ import scala.language.postfixOps
 import scala.util.{Random, Try}
 
 import com.esotericsoftware.kryo.Kryo
-import org.scalatest.Matchers
 
 import org.apache.spark.deploy.history.config._
 import org.apache.spark.internal.config._