[SPARK-4990][Deploy]to find default properties file, search SPARK_CONF_DIR first #3823

WangTaoTheTonic · 2014-12-29T08:20:32Z

https://issues.apache.org/jira/browse/SPARK-4990

SparkQA · 2014-12-29T08:22:28Z

Test build #24857 has started for PR 3823 at commit c5a85eb.

This patch merges cleanly.

srowen · 2014-12-29T09:16:16Z

bin/spark-submit

@@ -42,7 +42,10 @@ while (($#)); do
  shift
 done

-DEFAULT_PROPERTIES_FILE="$SPARK_HOME/conf/spark-defaults.conf"
+DEFAULT_PROPERTIES_FILE="$SPARK_CONF_DIR/spark-defaults.conf"


SparkSubmitArguments already ultimately handles this case, right? What does this fix?

Used for

if [[ "$SPARK_SUBMIT_DEPLOY_MODE" == "client" && -f "$SPARK_SUBMIT_PROPERTIES_FILE" ]]; then

Parse the properties file only if the special configs exist

contains_special_configs=$(
grep -e "spark.driver.extra_|spark.driver.memory" "$SPARK_SUBMIT_PROPERTIES_FILE" |
grep -v "^[[:space:]]_#"
)
if [ -n "$contains_special_configs" ]; then
export SPARK_SUBMIT_BOOTSTRAP_DRIVER=1
fi
fi

SparkQA · 2014-12-29T09:47:30Z

Test build #24857 has finished for PR 3823 at commit c5a85eb.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2014-12-29T09:47:33Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24857/
Test PASSed.

OopsOutOfMemory · 2014-12-29T13:17:47Z

Hi, @WangTaoTheTonic : )
This make sense to me, like hadoop also have HADOOP_CONF_DIR.

But I prefer to check if the SPARK_CONF_DIR directory is exists first, not only check the configuration file.
If there are many files under SPARK_CONF_DIR need to be added in spark-submit in the future, you will need to check each file is exists or not.

you can do it like :

if [ ! -d "$SPARK_CONF_DIR" ]; then

WangTaoTheTonic · 2014-12-29T13:39:34Z

@OopsOutOfMemory Thanks for your comment and I understand your concern.
Actually it doesn't matter that if SPARK_CONF_DIR does not exist here because we can use $SPARK_HOME/conf instead. And checking logic of properties file contains that of SPARK_CONF_DIR.
In other places of spark codes, we usually use a getOrElse logic to handle configuration.
It is easy to analyse when use got some specific config wrong and we'd better not to broke this tradition. :)

OopsOutOfMemory · 2014-12-29T14:30:22Z

Sorry, maybe here is a misunderstanding.
What I mean is to change the checking logic of properties file instead of __checking whether the SPARK_CONF_DIR isuser-specifcordefault` . But not to add an extra checking directory here .
Let me raise an example:

if [ ! -d "$SPARK_CONF_DIR" ]; then
   export SPARK_CONF_DIR = $SPARK_HOME/conf
fi
DEFAULT_PROPERTIES_FILE="$SPARK_CONF_DIR/spark-defaults.conf"
XXX_PROPERTIES_FILE = "$SPARK_CONF_DIR/xxx.conf

To check conf directory is more reasonable, because the key point is the SPARK_CONF_DIR. The original concern here is to change the path of SPARK_CONF_DIR but not only spark-default.conf.

analyse when use got some specific config wrong

We may add an extra warning for the key configuration file here. i.e If spark-deault.conf is missing or changed to a user-specific dir under conf_dir, we may raise an warning log to let user aware of this before submitting.

Currently, I think both of the two solutions is ok! : )

WangTaoTheTonic · 2014-12-30T01:40:38Z

@OopsOutOfMemory Ok I got what you mean. After checking the logic in SparkSubmitArguments.scala I do think that your solution is more reasonable. Thanks.

 val sparkHomeConfig = env.get("SPARK_HOME").map(sparkHome => s"${sparkHome}${sep}conf")
  val confDir = env.get("SPARK_CONF_DIR").orElse(sparkHomeConfig)

SparkQA · 2014-12-30T01:47:29Z

Test build #24875 has started for PR 3823 at commit 07b9ebf.

This patch merges cleanly.

SparkQA · 2014-12-30T02:17:31Z

Test build #24877 has started for PR 3823 at commit d8d3cb7.

This patch merges cleanly.

OopsOutOfMemory · 2014-12-30T02:51:33Z

bin/spark-submit

@@ -42,7 +42,10 @@ while (($#)); do
  shift
 done

-DEFAULT_PROPERTIES_FILE="$SPARK_HOME/conf/spark-defaults.conf"
+if [ ! -d "$SPARK_CONF_DIR" ]; then
+  SPARK_CONF_DIR="$SPARK_HOME/conf"


I recommend to add export keyword to make the SPARK_CONF_DIR global :)

export SPARK_CONF_DIR="$SPARK_HOME/conf"

Just like SPARK_HOME, it's a global variable.

SparkQA · 2014-12-30T02:53:30Z

Test build #24875 has finished for PR 3823 at commit 07b9ebf.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2014-12-30T02:53:34Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24875/
Test FAILed.

SparkQA · 2014-12-30T03:38:56Z

Test build #24877 has finished for PR 3823 at commit d8d3cb7.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2014-12-30T03:39:00Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24877/
Test PASSed.

SparkQA · 2014-12-30T05:57:30Z

Test build #24883 has started for PR 3823 at commit 55300bc.

This patch merges cleanly.

SparkQA · 2014-12-30T07:20:53Z

Test build #24883 has finished for PR 3823 at commit 55300bc.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2014-12-30T07:20:57Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24883/
Test PASSed.

andrewor14 · 2015-01-08T20:31:31Z

LGTM, but this seems to have a conflict from your other patch now. Could you rebase to master?

WangTaoTheTonic · 2015-01-09T01:54:12Z

@andrewor14 Ok, rebase done.

SparkQA · 2015-01-09T01:57:40Z

Test build #25286 has started for PR 3823 at commit 4cc7f34.

This patch merges cleanly.

SparkQA · 2015-01-09T03:05:54Z

Test build #25286 has finished for PR 3823 at commit 4cc7f34.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-01-09T03:05:57Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25286/
Test PASSed.

andrewor14 · 2015-01-09T17:43:59Z

bin/spark-submit

@@ -44,7 +44,10 @@ while (($#)); do
  shift
 done

-DEFAULT_PROPERTIES_FILE="$SPARK_HOME/conf/spark-defaults.conf"
+if [ ! -d "$SPARK_CONF_DIR" ]; then


actually, maybe this should be "if not defined" instead of "if not exists". Otherwise if the user sets SPARK_CONF_DIR to a directory that doesn't exist then this will silently use a different config.

i.e.

if [ -z "$SPARK_CONF_DIR" ]; then export SPARK_CONF_DIR="$SPARK_HOME/conf" fi

SparkQA · 2015-01-10T00:22:39Z

Test build #25347 has started for PR 3823 at commit b1ab402.

This patch merges cleanly.

WangTaoTheTonic · 2015-01-10T00:26:05Z

@andrewor14 Ok, that makes sense. I changed not exist to not defined in windows cmd too.

SparkQA · 2015-01-10T00:27:48Z

Test build #25348 has started for PR 3823 at commit 133c43e.

This patch merges cleanly.

andrewor14 · 2015-01-10T01:09:53Z

Ok I'm merging this into master since tests are irrelevant here thanks.

SparkQA · 2015-01-10T01:29:56Z

Test build #25347 has finished for PR 3823 at commit b1ab402.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-01-10T01:30:00Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25347/
Test PASSed.

SparkQA · 2015-01-10T01:34:00Z

Test build #25348 has finished for PR 3823 at commit 133c43e.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-01-10T01:34:03Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25348/
Test PASSed.

to find default properties file, search SPARK_CONF_DIR first

c5a85eb

WangTaoTheTonic changed the title ~~[SPARK-4990]to find default properties file, search SPARK_CONF_DIR first~~ [SPARK-4990][Deploy]to find default properties file, search SPARK_CONF_DIR first Dec 29, 2014

srowen reviewed Dec 29, 2014
View reviewed changes

check SPARK_CONF_DIR instead of checking properties file

07b9ebf

remove blank line

d8d3cb7

OopsOutOfMemory reviewed Dec 30, 2014
View reviewed changes

use export to make it global

55300bc

rebase

4cc7f34

andrewor14 reviewed Jan 9, 2015
View reviewed changes

Update spark-submit

b1ab402

Update spark-submit2.cmd

133c43e

asfgit closed this in 8782eb9 Jan 10, 2015

[SPARK-4990][Deploy]to find default properties file, search SPARK_CONF_DIR first #3823

[SPARK-4990][Deploy]to find default properties file, search SPARK_CONF_DIR first #3823

Uh oh!

Conversation

WangTaoTheTonic commented Dec 29, 2014

Uh oh!

SparkQA commented Dec 29, 2014

Uh oh!

srowen Dec 29, 2014

Choose a reason for hiding this comment

Uh oh!

WangTaoTheTonic Dec 29, 2014

Choose a reason for hiding this comment

Parse the properties file only if the special configs exist

Uh oh!

SparkQA commented Dec 29, 2014

Uh oh!

AmplabJenkins commented Dec 29, 2014

Uh oh!

OopsOutOfMemory commented Dec 29, 2014

Uh oh!

WangTaoTheTonic commented Dec 29, 2014

Uh oh!

OopsOutOfMemory commented Dec 29, 2014

Uh oh!

WangTaoTheTonic commented Dec 30, 2014

Uh oh!

SparkQA commented Dec 30, 2014

Uh oh!

SparkQA commented Dec 30, 2014

Uh oh!

OopsOutOfMemory Dec 30, 2014

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 30, 2014

Uh oh!

AmplabJenkins commented Dec 30, 2014

Uh oh!

SparkQA commented Dec 30, 2014

Uh oh!

AmplabJenkins commented Dec 30, 2014

Uh oh!

SparkQA commented Dec 30, 2014

Uh oh!

SparkQA commented Dec 30, 2014

Uh oh!

AmplabJenkins commented Dec 30, 2014

Uh oh!

andrewor14 commented Jan 8, 2015

Uh oh!

WangTaoTheTonic commented Jan 9, 2015

Uh oh!

SparkQA commented Jan 9, 2015

Uh oh!

SparkQA commented Jan 9, 2015

Uh oh!

AmplabJenkins commented Jan 9, 2015

Uh oh!

andrewor14 Jan 9, 2015

Choose a reason for hiding this comment

Uh oh!

andrewor14 Jan 9, 2015

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 10, 2015

Uh oh!

WangTaoTheTonic commented Jan 10, 2015

Uh oh!

SparkQA commented Jan 10, 2015

Uh oh!

andrewor14 commented Jan 10, 2015

Uh oh!

SparkQA commented Jan 10, 2015

Uh oh!

AmplabJenkins commented Jan 10, 2015

Uh oh!

SparkQA commented Jan 10, 2015

Uh oh!

AmplabJenkins commented Jan 10, 2015

Uh oh!