Skip to content

SPARK-2058: Overriding config from SPARK_HOME with SPARK_CONF_DIR #997

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.

Sign up for GitHub

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

Closed
EugenCepoi wants to merge 7 commits into from
Closed

SPARK-2058: Overriding config from SPARK_HOME with SPARK_CONF_DIR #997

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5ede2c0
SPARK-2058: use spark-defaults.conf from SPARK_CONF_DIR when defined
Jun 6, 2014
96e3fdd
SPARK-2058: override the classpath with SPARK_CONF_DIR
Jun 6, 2014
69d337e
fixing typo in compute-classpath.sh
Jun 6, 2014
99a4341
fixing inverted code to set SPARK_CONF_DIR in compute-classpath script
Jun 12, 2014
5357369
using Utils.deleteRecursively to clean the generated conf, replace ha…
Jun 13, 2014
d2d1543
using guava Files instead of one from java 7
Jun 13, 2014
186c975
updating configuration doc with SPARK_CONF_DIR
EugenCepoi Jun 14, 2014
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
7 changes: 7 additions & 0 deletions bin/compute-classpath.cmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,13 @@ if exist "%FWDIR%conf\spark-env.cmd" call "%FWDIR%conf\spark-env.cmd"

rem Build up classpath
set CLASSPATH=%FWDIR%conf

if "x%SPARK_CONF_DIR%"!="x" (
rem If SPARK_CONF_DIR is defined give it preference over default conf in spark home

set CLASSPATH=%SPARK_CONF_DIR%;%CLASSPATH%
)

if exist "%FWDIR%RELEASE" (
for %%d in ("%FWDIR%lib\spark-assembly*.jar") do (
set ASSEMBLY_JAR=%%d
Expand Down
5 changes: 5 additions & 0 deletions bin/compute-classpath.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ FWDIR="$(cd `dirname $0`/..; pwd)"
# Build up classpath
CLASSPATH="$SPARK_CLASSPATH:$SPARK_SUBMIT_CLASSPATH:$FWDIR/conf"

# If SPARK_CONF_DIR is defined give it preference over default conf in spark home
if [ -n "$SPARK_CONF_DIR" ]; then
CLASSPATH="$SPARK_CONF_DIR:$CLASSPATH"
Copy link
Contributor

@vanzin vanzin Jun 11, 2014

Choose a reason for hiding this comment

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

Does the configuration dir need to be added to the classpath? The code, at least the part you're modifying below, doesn't seem to require that.

Copy link
Contributor Author

@EugenCepoi EugenCepoi Jun 12, 2014

Choose a reason for hiding this comment

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

Indeed it's not used in the SparkSubmit, but this is done in order to provide the user defined config to the other components such as logging, Scheduler and metrics.

Copy link
Contributor

@lianhuiwang lianhuiwang Jul 18, 2014

Choose a reason for hiding this comment

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

also i think in standalone mode,worker need to use config. so the classpath is useful.

fi

ASSEMBLY_DIR="$FWDIR/assembly/target/scala-$SCALA_VERSION"

if [ -n "$JAVA_HOME" ]; then
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ object SparkSubmit {
private val PYSPARK_SHELL = "pyspark-shell"

def main(args: Array[String]) {
val appArgs = new SparkSubmitArguments(args)
val appArgs = new SparkSubmitArguments(args, sys.env)
Copy link
Contributor

@andrewor14 andrewor14 Aug 25, 2014

Choose a reason for hiding this comment

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

No need to pass sys.env to SparkSubmitArguments. You can just get it from there directly since it's the same JVM

if (appArgs.verbose) {
printStream.println(appArgs)
}
Expand Down
13 changes: 8 additions & 5 deletions core/src/main/scala/org/apache/spark/deploy/SparkSubmitArguments.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import org.apache.spark.util.Utils
/**
* Parses and encapsulates arguments from the spark-submit script.
*/
private[spark] class SparkSubmitArguments(args: Seq[String]) {
private[spark] class SparkSubmitArguments(args: Seq[String], env: Map[String, String] = sys.env) {
Copy link
Contributor

@andrewor14 andrewor14 Aug 25, 2014

Choose a reason for hiding this comment

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

No need to accept an extra argument here. This will always read from the JVM directly anyway.

Copy link
Contributor

@andrewor14 andrewor14 Aug 25, 2014

Choose a reason for hiding this comment

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

I see, you use this for tests. Then it makes sense to add a comment here to explain that.

var master: String = null
var deployMode: String = null
var executorMemory: String = null
Expand Down Expand Up @@ -83,9 +83,12 @@ private[spark] class SparkSubmitArguments(args: Seq[String]) {

// Use common defaults file, if not specified by user
if (propertiesFile == null) {
sys.env.get("SPARK_HOME").foreach { sparkHome =>
val sep = File.separator
val defaultPath = s"${sparkHome}${sep}conf${sep}spark-defaults.conf"
val sep = File.separator
val sparkHomeConfig = env.get("SPARK_HOME").map(sparkHome => s"${sparkHome}${sep}conf")

// give preference to user defined conf over the one in spark home
env.get("SPARK_CONF_DIR").orElse(sparkHomeConfig).foreach { configPath =>
val defaultPath = s"${configPath}${sep}spark-defaults.conf"
val file = new File(defaultPath)
if (file.exists()) {
propertiesFile = file.getAbsolutePath
Expand Down Expand Up @@ -161,7 +164,7 @@ private[spark] class SparkSubmitArguments(args: Seq[String]) {
}

if (master.startsWith("yarn")) {
val hasHadoopEnv = sys.env.contains("HADOOP_CONF_DIR") || sys.env.contains("YARN_CONF_DIR")
val hasHadoopEnv = env.contains("HADOOP_CONF_DIR") || env.contains("YARN_CONF_DIR")
if (!hasHadoopEnv && !Utils.isTesting) {
throw new Exception(s"When running with master '$master' " +
"either HADOOP_CONF_DIR or YARN_CONF_DIR must be set in the environment.")
Expand Down
35 changes: 34 additions & 1 deletion core/src/test/scala/org/apache/spark/deploy/SparkSubmitSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

package org.apache.spark.deploy

import java.io.{File, OutputStream, PrintStream}
import java.io._

import scala.collection.mutable.ArrayBuffer

Expand Down Expand Up @@ -264,6 +264,21 @@ class SparkSubmitSuite extends FunSuite with ShouldMatchers {
runSparkSubmit(args)
}

test("SPARK_CONF_DIR overrides spark-defaults.conf") {
forConfDir(Map("spark.executor.memory" -> "2.3g")) { path =>
val unusedJar = TestUtils.createJarWithClasses(Seq.empty)
val args = Seq(
"--class", SimpleApplicationTest.getClass.getName.stripSuffix("$"),
"--name", "testApp",
"--master", "local",
unusedJar.toString)
val appArgs = new SparkSubmitArguments(args, Map("SPARK_CONF_DIR" -> path))
assert(appArgs.propertiesFile != null)
assert(appArgs.propertiesFile.startsWith(path))
appArgs.executorMemory should be ("2.3g")
}
}

// NOTE: This is an expensive operation in terms of time (10 seconds+). Use sparingly.
def runSparkSubmit(args: Seq[String]): String = {
val sparkHome = sys.env.get("SPARK_HOME").orElse(sys.props.get("spark.home")).get
Expand All @@ -272,6 +287,24 @@ class SparkSubmitSuite extends FunSuite with ShouldMatchers {
new File(sparkHome),
Map("SPARK_TESTING" -> "1", "SPARK_HOME" -> sparkHome))
}

def forConfDir(defaults: Map[String, String]) (f: String => Unit) = {
val tmpDir = new File("TMP_SPARK_CONF_DIR")
Copy link
Contributor

@vanzin vanzin Jun 12, 2014

Choose a reason for hiding this comment

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

Use Files.createTempDir(), and delete it when the test is done (using scalatest's BeforeAndAfter); there are several examples of this in other tests.

Copy link
Contributor

@vanzin vanzin Jun 12, 2014

Choose a reason for hiding this comment

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

Just a clarification: I see you're deleting the directory below, so BeforeAndAfter does not apply here. But still I'd prefer to use Files.createTempDir() instead of a hardcoded name. Just for paranoia.

tmpDir.mkdir()

val defaultsConf = new File(tmpDir.getAbsolutePath, "spark-defaults.conf")
val writer = new OutputStreamWriter(new FileOutputStream(defaultsConf))
for ((key, value) <- defaults) writer.write(key + " " + value)
Copy link
Contributor

@vanzin vanzin Jun 12, 2014

Choose a reason for hiding this comment

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

Don't you need a new line at the end there?

s"$key=$value\n"

writer.flush()
Copy link
Contributor

@vanzin vanzin Jun 12, 2014

Choose a reason for hiding this comment

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

Sort of redundant with close() on the next line.

writer.close()

try {
f(tmpDir.getAbsolutePath)
} finally {
defaultsConf.delete()
Copy link
Member

@srowen srowen Jun 12, 2014

Choose a reason for hiding this comment

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

While we're here, this can be one call to Utils.deleteRecursively(tmpDir)

tmpDir.delete()
}
}
}

object JarCreationTest {
Expand Down