[SPARK-3393] [SQL] Align the log4j configuration for Spark & SparkSQLCLI

[chenghao-intel]

User may be confused for the HQL logging & configurations, we'd better provide a default templates.

Both files are copied from Hive.

[chenghao-intel]

@liancheng, this is not change of any code, but kind of hint for the user of HQL, I was asked couple of times for the configuration files, hope it helpful. Any comments? :)

[liancheng]

The license header is not exactly the same as other files, I'm not sure but maybe RAT may reject this one.

[liancheng]

I know that hive-log4j.properties.template is copied from Hive, but this file really makes me itchy...

Besides that, I think these are generally good to have, thanks!

[liancheng]

Remove this line

[chenghao-intel]

Thanks for reviewing this, you're right, the log4j configuration template mainly for Hive usage (particularly for MapReduce), I've updated the code and keep it simple for demo purpose.

I also checked the other template files under conf, seems the licence header is not necessary for .properties files. Let's do it the same way?

[liancheng]

Not all .properties files are ignored by RAT, it's controlled by .rat-excludes, you may add hive-log4j.properties.template there.

[chenghao-intel]

Thank you @liancheng . I didn't notice that file previously. :)

test this please.

[marmbrus]

Its not really clear to me what this buys over the log4j.template that is already there. I'm also not sure what the xsl file is for. Finally an empty hive-site.xml file only saves you from adding <configuration> which most users are probably going to copy from wherever they found the hive options they want at. Given that I think this might be more clutter than its worth. What do you think?

[chenghao-intel]

@marmbrus those files are just a hint for users if they want to change some of the default settings. Probably not everybody knows what files exactly they should put under the folder conf/
e.g.
hive-log4j.properties.template => hive-log4j.properties (not log4j.properties see https://github.com/apache/spark/blob/master/sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLCLIDriver.scala#L81)
hive-site.xml.template is for people familiar with Shark, they probably confused with the Hive dependency means. In Shark, Hive is also required to be installed, and the hive-site.xml is placed under $HIVE/conf.
configuration.xsl is the xml style file for display the hive-site.xml on browser.(http://www.w3.org/Style/XSL/WhatIsXSL.html)

I agree it's probably more clutter if we add those 3 files, but at least we should keep the hive-log4j.properties.template, or we have to change the code SparkSQLCLIDriver.scala to load log4j.properties instead of the hive-log4j.properties. What do you think?

[liancheng]

I'd agree that at least hive-log4j.properties.template should be good to have. Partly because I myself had once been confused by this a lot...

[chenghao-intel]

Thank you @liancheng , I've updated the code.

[SparkQA]

QA tests have started for PR 2263 at commit e027d23.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 2263 at commit e027d23.

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

[marmbrus]

Oh, okay thanks for explaining this! My inclination would be to update our code to read from log4j.properties instead, unless there is some compelling reason not to. Otherwise, can we maybe add a comment to the template file that explains what it is for and why its needed in addition to the standard config file?

[chenghao-intel]

2 logj4j properties files does make people confusing, I will try to update the code, let's see if we can find a simple way to reuse the log4j.properties.

[chenghao-intel]

@liancheng @marmbrus I've updated the code by removing the log4j initialization code in SparkSQLCLIDriver.scala, now it will load the log4j.properties by default, and it works fine in my cluster testing.

[SparkQA]

QA tests have started for PR 2263 at commit 53bffa9.

• This patch merges cleanly.

[liancheng]

Does this mean Hive log will never be initialized? Won't this prevent us from checking Hive logs?

[SparkQA]

QA tests have finished for PR 2263 at commit 53bffa9.

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

[chenghao-intel]

The Hive LogUtils just load the specified .properties file for Log4J, see https://github.com/apache/hive/blob/branch-0.12/common/src/java/org/apache/hadoop/hive/common/LogUtils.java#L117-L126

If we remove the logic in SparkSQLCliDriver, Log4J will load the log4j.properties under the classpath automatically.

[marmbrus]

@liancheng so is this ready to merge?

[marmbrus]

@liancheng ping

[liancheng]

Sorry, this LGTM.

[marmbrus]

Thanks! Merged to master.