[SPARK-11105][yarn] Distribute log4j.properties to executors #9118

vundela · 2015-10-14T19:09:55Z

Currently log4j.properties file is not uploaded to executor's which is leading them to use the default values. This fix will make sure that file is always uploaded to distributed cache so that executor will use the latest settings.

If user specifies log configurations through --files then executors will be picking configs from --files instead of $SPARK_CONF_DIR/log4j.properties

vundela · 2015-10-14T19:13:15Z

@vanzin can you please review the pull request.

markgrover · 2015-10-14T19:41:50Z

yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala

Nit: I'd prefer vendor neutral vocabulary here.

Thanks for the suggestion, done.

vanzin · 2015-10-14T20:36:19Z

yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala

As Mark mentioned, this can throw a NPE.

vanzin · 2015-10-14T22:43:14Z

yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala

So, a more scala-y way of doing this would be:

val log4jConf = oldLog4jConf.orElse(Option(getClass.getResource("/log4j.properties")).map(_.toString))

Also, it might be better to use Utils.getContextOrSparkClassLoader.getResource instead, so that it picks up the user's log4j.properties it they embed it in their app's jar.

vanzin · 2015-10-15T00:15:21Z

ok to test

vanzin · 2015-10-15T00:15:55Z

LGTM pending tests.

SparkQA · 2015-10-15T00:46:03Z

Test build #43756 has finished for PR 9118 at commit 9669a32.

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

markgrover · 2015-10-15T14:48:41Z

LGTM too, the tests seem to have passed as well.

srowen · 2015-10-15T15:06:59Z

yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala

I might be over-estimating the intended scope, but this is not the only place that log4j.properties can occur. Generally callers can set -Dlog4j.configuration=... to set its location.

Sure, but we're trying to make the default case easier. If the user is messing with log4j.configuration then he probably knows what he's doing, and this change would not affect that.

vanzin · 2015-10-15T16:24:15Z

So, I thought about this a little bit more and I think it would be worth it to put that file in createConfArchive instead. That way, it's more efficient, since it's one less tiny file we're uploading to HDFS before starting the job. It also would maintain the current behavior of not overriding --files, since the configuration dir is added later in the classpath.

Could you try that? Thanks!

SparkQA · 2015-10-19T16:31:35Z

Test build #43929 has finished for PR 9118 at commit 187eb99.

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

tgravescs · 2015-10-19T18:44:06Z

This isn't really a bug as it was kind of designed this way. Originally you had to either set SPARK_LOG4J_CONF (which does SPARK_LOG4J_CONF still work properly?) or you had to upload it with the --files.

I'm definitely fine with having this happen automatically but we need to update the documentation at http://spark.apache.org/docs/latest/running-on-yarn.html. It talks about using the --files and setting the java options.

I assume --files still works because it will show up on the classpath first because it looks like it will still upload the one from the conf dir?

vundela · 2015-10-19T19:56:27Z

@tgravescs

Thanks for the review. Yes, it make sense to update the documentation.

vanzin · 2015-10-19T22:31:15Z

yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala

Technically, you should only look at this URL if its protocol is file, otherwise even if the path matches a local file, it wouldn't be correct.

Also, style:

.map { path => // code }

SparkQA · 2015-10-19T22:41:51Z

Test build #43943 has finished for PR 9118 at commit 39c75a2.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2015-10-20T00:06:13Z

Test build #43952 has finished for PR 9118 at commit e1e474a.

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

vanzin · 2015-10-20T01:21:06Z

yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala

style: url => goes on the previous line.

SparkQA · 2015-10-20T02:26:11Z

Test build #43958 has finished for PR 9118 at commit b97a2b5.

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

tgravescs · 2015-10-20T14:37:12Z

yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala

Why does --files work? Is it just because it gets put on classpath first?

Yes, thats correct and also in this case user knows what he is doing and his intension is to overwrite the default behavior

tgravescs · 2015-10-20T14:37:36Z

@vundela Does SPARK_LOG4J_CONF still work?

vundela · 2015-10-20T14:42:33Z

@tgravescs Yes, it still works and user see a deprecated message.

tgravescs · 2015-10-20T15:41:18Z

changes lgtm. @vanzin are you ok with these?

markgrover · 2015-10-20T17:44:38Z

I have one more noob question. Let's say I have a 4 node hadoop/yarn cluster and a 5th node, I am driving a yarn-client app from. I use puppet to set up my log4j configuration (in $SPARK_CONF_DIR of each of the executor nodes) and it's set up differently on the 4 cluster nodes, vs. the 5th driver node. After this change, would the 5th node's log4j configuration always override my log4j configuration on the 4 executor nodes? Would that come as a surprise to some users?

vanzin · 2015-10-20T18:11:57Z

@markgrover you can't have that scenario on YARN because when running Spark through YARN there is no "cluster configuration". Everything is configured based on the gateway node's configuration. You don't even need Spark jars available in the cluster.

The change LGTM, I'll merge to master.

markgrover · 2015-10-20T18:30:09Z

Got it, thanks!

…ally with distributed cache ## What changes were proposed in this pull request? 1. Currently log4j which uses distributed cache only adds to AM's classpath, not executor's, this is introduced in #9118, which breaks the original meaning of that PR, so here add log4j file to the classpath of both AM and executors. 2. Automatically upload metrics.properties to distributed cache, so that it could be used by remote driver and executors implicitly. ## How was this patch tested? Unit test and integration test is done. Author: jerryshao <sshao@hortonworks.com> Closes #11885 from jerryshao/SPARK-14062.

[SPARK-11105] Distribute log4j.properties to executors

cc09f86

markgrover reviewed Oct 14, 2015
View reviewed changes

Changing vocabulary

2772c1d

vanzin reviewed Oct 14, 2015
View reviewed changes

Srinivasa Reddy Vundela added 3 commits October 14, 2015 14:17

Fixing the possible NullPointerIssue

4ffcb3a

-a

f007acf

-a

85aecef

vanzin reviewed Oct 14, 2015
View reviewed changes

Making changes as suggested by Marcelo

9669a32

srowen reviewed Oct 15, 2015
View reviewed changes

vundela added 2 commits October 19, 2015 08:54

Uploading log4j conf through archived files

2e34edc

Indentation

187eb99

Uploading documentation to reflect the new behaviour

39c75a2

vanzin reviewed Oct 19, 2015
View reviewed changes

Making changes as per suggestion

e1e474a

vanzin reviewed Oct 20, 2015
View reviewed changes

yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala Outdated

Copy link

Contributor

vanzin Oct 20, 2015

Choose a reason for hiding this comment

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

style: url => goes on the previous line.

Style change

b97a2b5

tgravescs reviewed Oct 20, 2015
View reviewed changes

asfgit closed this in 2f6dd63 Oct 20, 2015

jerryshao mentioned this pull request Mar 23, 2016

[SPARK-14062][Yarn] Fix log4j and upload metrics.properties automatically with distributed cache #11885

Closed

[SPARK-11105][yarn] Distribute log4j.properties to executors #9118

[SPARK-11105][yarn] Distribute log4j.properties to executors #9118

Uh oh!

Conversation

vundela commented Oct 14, 2015

Uh oh!

vundela commented Oct 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vanzin commented Oct 15, 2015

Uh oh!

vanzin commented Oct 15, 2015

Uh oh!

SparkQA commented Oct 15, 2015

Uh oh!

markgrover commented Oct 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vanzin commented Oct 15, 2015

Uh oh!

SparkQA commented Oct 19, 2015

Uh oh!

tgravescs commented Oct 19, 2015

Uh oh!

vundela commented Oct 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 19, 2015

Uh oh!

SparkQA commented Oct 20, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 20, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgravescs commented Oct 20, 2015

Uh oh!

vundela commented Oct 20, 2015

Uh oh!

tgravescs commented Oct 20, 2015

Uh oh!

markgrover commented Oct 20, 2015

Uh oh!

vanzin commented Oct 20, 2015

Uh oh!

markgrover commented Oct 20, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants