[SPARK-9826][Core] Fix cannot use custom classes in log4j.properties

[michellemay]

Refactor Utils class and create ShutdownHookManager.

NOTE: Wasn't able to run /dev/run-tests on windows machine.
Manual tests were conducted locally using custom log4j.properties file with Redis appender and logstash formatter (bundled in the fat-jar submitted to spark)

ex:
log4j.rootCategory=WARN,console,redis
log4j.appender.console=org.apache.log4j.ConsoleAppender
log4j.appender.console.target=System.err
log4j.appender.console.layout=org.apache.log4j.PatternLayout
log4j.appender.console.layout.ConversionPattern=%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n

log4j.logger.org.eclipse.jetty=WARN
log4j.logger.org.eclipse.jetty.util.component.AbstractLifeCycle=ERROR
log4j.logger.org.apache.spark.repl.SparkIMain$exprTyper=INFO
log4j.logger.org.apache.spark.repl.SparkILoop$SparkILoopInterpreter=INFO
log4j.logger.org.apache.spark.graphx.Pregel=INFO

log4j.appender.redis=com.ryantenney.log4j.FailoverRedisAppender
log4j.appender.redis.endpoints=hostname:port
log4j.appender.redis.key=mykey
log4j.appender.redis.alwaysBatch=false
log4j.appender.redis.layout=net.logstash.log4j.JSONEventLayoutV1

[vanzin]

This line is too long and will fail style checks.

[michellemay]

How do you suggest to split this line up? I'm not too much familiar with scala best practices and style.

[vanzin]

The original reason (from what I remember) to hide this under the Utils class was to avoid exposing the ShutdownHookManager, which clashes with a Hadoop class with the same name (different package though). I guess since Spark code should not use the Hadoop one, it's ok to use the same name.

The change looks ok, but let's see what tests say.

[vanzin]

ok to test

[vanzin]

Jenkins, test this please.

[vanzin]

ok to test

[michellemay]

Where do we see test results ?

[SparkQA]

Test build #1469 has finished for PR 8109 at commit cdcd1f6.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds no public classes.

[vanzin]

@michellemay the last build has a link to the results (it's failing the style checks).

[SparkQA]

Test build #40624 has finished for PR 8109 at commit 2e2a3ee.

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

[michellemay]

@vanzin about your comment on ShutdownHookManager, should I rename it?

Also, while refactoring calls to Utils.addShutdownHook, I've seen instances of calls to Runtime.getRuntime.addShutdownHook.. Should I replace them with calls to the new class?

org\apache\spark\deploy\ExternalShuffleService.scala:126
org\apache\spark\deploy\mesos\MesosClusterDispatcher.scala:113
org\apache\spark\storage\ExternalBlockStore.scala:181

[vanzin]

about your comment on ShutdownHookManager, should I rename it?

No, the name is fine. I was just giving some historical context.

I've seen instances of calls to Runtime.getRuntime.addShutdownHook..

From a quick look, it seems that at least the one in ExternalBlockStore should be changed; the other two seem to be separate processes (i.e. they are not part of a Spark application) so that's questionable. But I'd say leave it for a different change (maybe file a separate bug so it doesn't fall through the cracks).

[vanzin]

nit: sorting order

[vanzin]

LGTM, just have style nits left. I'll let this sit for a little bit to see if anyone else has comments.

[vanzin]

Alright, merging. Will fix the imports in the process.

[vanzin]

Merged to master, branch-1.5 and branch-1.4. Thanks!

I fixed some minor conflicts in the 1.4 port (mostly import statements), made sure the build passed, and ran core/ unit tests locally.

[michellemay]

Awesome! Thanks Marcelo for your implication!