Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-24337][Core] Improve error messages for Spark conf values #21454

Closed
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 60 additions & 21 deletions core/src/main/scala/org/apache/spark/SparkConf.scala
Original file line number Diff line number Diff line change
Expand Up @@ -265,108 +265,121 @@ 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually avoid abbreviation in the documentation though. can't -> cannot.

*/
def getTimeAsSeconds(key: String): Long = {
def getTimeAsSeconds(key: String): Long = catchIllegalValue(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 = catchIllegalValue(key) {
Utils.timeStringAsSeconds(get(key, defaultValue))
}

/**
* 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 = catchIllegalValue(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 = catchIllegalValue(key) {
Utils.timeStringAsMs(get(key, defaultValue))
}

/**
* 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 = catchIllegalValue(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 = catchIllegalValue(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 = catchIllegalValue(key) {
Utils.byteStringAsBytes(get(key, defaultValue + "B"))
}

/**
* 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 = catchIllegalValue(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 = catchIllegalValue(key) {
Utils.byteStringAsKb(get(key, defaultValue))
}

/**
* 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 = catchIllegalValue(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 = catchIllegalValue(key) {
Utils.byteStringAsMb(get(key, defaultValue))
}

/**
* 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 = catchIllegalValue(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 = catchIllegalValue(key) {
Utils.byteStringAsGb(get(key, defaultValue))
}

Expand Down Expand Up @@ -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 = catchIllegalValue(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 a long
*/
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 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 a double
*/
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 */
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 a boolean
*/
def getBoolean(key: String, defaultValue: Boolean): Boolean = catchIllegalValue(key) {
getOption(key).map(_.toBoolean).getOrElse(defaultValue)
}

Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get*() .. ?

* any [[NumberFormatException]] or [[IllegalArgumentException]] and re-raises it with the
* incorrectly configured key in the exception message.
*/
private def catchIllegalValue[T](key: String)(getValue: => T): T = {
try {
getValue
} catch {
case e @ (_ : NumberFormatException | _ : IllegalArgumentException) =>
throw new IllegalArgumentException(s"Illegal value for config key $key", e)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Use s"Illegal value for config key $key: ${e.getMessage}" so that the error message of the cause can be easy to find.
  • Avoid to change the exception type here. Let's just create the same exception type for each case.

}
}

/**
* 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.
Expand Down
36 changes: 35 additions & 1 deletion core/src/test/scala/org/apache/spark/SparkConfSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,16 @@ 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._
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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would do with Matchers here.

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
Expand Down Expand Up @@ -339,6 +341,38 @@ class SparkConfSuite extends SparkFunSuite with LocalSparkContext with ResetSyst
}
}

val defaultIllegalValue = "SomeIllegalValue"
val illegalValueTests : 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))
)

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()
conf.set(key, "SomeInvalidValue")
val thrown = the [IllegalArgumentException] thrownBy {
getValue(conf, key)
}
thrown.getMessage should include (key)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we stick to assert syntax?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Should we change the above block to a try ... catch, or leave it as is? I'm not sure on the recommended style here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PenguinToast something like this

val thrown = intercept[IllegalArgumentException] {
        getValue(conf, key)
 }
assert(thrown.getMessage.contains(key))

}
}
}

class Class1 {}
Expand Down