[SPARK-19664][SQL]put hive.metastore.warehouse.dir in hadoopconf to overwrite its original value #16996

windpiger · 2017-02-20T05:08:53Z

What changes were proposed in this pull request?

In SPARK-15959, we bring back the hive.metastore.warehouse.dir , while in the logic, when use the value of spark.sql.warehouse.dir to overwrite hive.metastore.warehouse.dir , it set it to sparkContext.conf which does not overwrite the value is hadoopConf, I think it should put in sparkContext.hadoopConfiguration and overwrite the original value of hadoopConf

https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/internal/SharedState.scala#L64

How was this patch tested?

N/A

…verwrite its original value

SparkQA · 2017-02-20T06:14:12Z

Test build #73144 has finished for PR 16996 at commit 92c1452.

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

SparkQA · 2017-02-20T07:02:40Z

Test build #73152 has started for PR 16996 at commit ac0a1c6.

windpiger · 2017-02-20T08:06:05Z

retest this please

SparkQA · 2017-02-20T10:00:35Z

Test build #73155 has finished for PR 16996 at commit ac0a1c6.

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

windpiger · 2017-02-20T10:05:19Z

cc @cloud-fan

gatorsmile · 2017-02-20T18:38:55Z

sql/core/src/main/scala/org/apache/spark/sql/internal/SharedState.scala

      // When neither spark.sql.warehouse.dir nor hive.metastore.warehouse.dir is set,
      // we will set hive.metastore.warehouse.dir to the default value of spark.sql.warehouse.dir.
      val sparkWarehouseDir = sparkContext.conf.get(WAREHOUSE_PATH)
+      logInfo(s"${WAREHOUSE_PATH.key} is set, Setting " +


Please remove "${WAREHOUSE_PATH.key} is set,

gatorsmile · 2017-02-20T18:39:57Z

In your PR title, verwrite -> overwrite

gatorsmile · 2017-02-20T18:41:05Z

Why we need to overwrite its original value? I thought we will not use that value any more.

windpiger · 2017-02-21T03:53:05Z

this is a minor improvement, it is better to keep the same value between sparkConf and hadoopConf, and add some loginfo for user debug.

gatorsmile · 2017-02-21T04:27:31Z

cc @yhuai who did the original change. I am not sure whether we need to overwrite the original value of hadoopConf, although the change does not hurt anything IMO.

SparkQA · 2017-02-21T05:49:02Z

Test build #73198 has finished for PR 16996 at commit 91b9fd2.

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

windpiger · 2017-02-22T05:00:10Z

@yhuai could you help to review this？ thanks~

cloud-fan · 2017-02-23T01:53:36Z

sql/core/src/main/scala/org/apache/spark/sql/internal/SharedState.scala

  val warehousePath = {
    val configFile = Utils.getContextOrSparkClassLoader.getResource("hive-site.xml")
    if (configFile != null) {
+      logInfo(s"load config from hive-site.xml $configFile")


nit: loading hive config file: $configFile

cloud-fan · 2017-02-23T01:55:56Z

sql/core/src/main/scala/org/apache/spark/sql/internal/SharedState.scala

      // When neither spark.sql.warehouse.dir nor hive.metastore.warehouse.dir is set,
      // we will set hive.metastore.warehouse.dir to the default value of spark.sql.warehouse.dir.
      val sparkWarehouseDir = sparkContext.conf.get(WAREHOUSE_PATH)
+      logInfo(s"Setting hive.metastore.warehouse.dir ($hiveWarehouseDir) to the value of " +


nit: hive.metastore.warehouse.dir is not set, setting it to the value of ${WAREHOUSE_PATH.key}

when we hit this condition because we provide a WAREHOUSE_PATH , maybe the hive.metastore.warehouse.dir also have been set

($hiveWarehouseDir) -> ('$hiveWarehouseDir')

cloud-fan · 2017-02-23T01:56:11Z

sql/core/src/main/scala/org/apache/spark/sql/internal/SharedState.scala

+      if (hiveWarehouseDir != null) {
+        sparkContext.hadoopConfiguration.set("hive.metastore.warehouse.dir", sparkWarehouseDir)
+      }
      sparkContext.conf.set("hive.metastore.warehouse.dir", sparkWarehouseDir)


do we still need to do this?

I removed hive.metastore.warehouse.dir in sparkConf above commit ,but some tests failed, so I revert this logic, let me dig it more.

I look into the code, and make some tests.
The hive.metastore.warehouse.dir in sparkConf still take effect in Spark, it is not useless.
The reason is that:

when we run spark with HiveEnabled, it will create ShareState

when create ShareState, it will create a HiveExternalCatalog
https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/internal/SharedState.scala#L85
3.when create HiveExternalCatalog, it will Create HiveClientImpl by HiveUtils
https://github.com/apache/spark/blob/master/sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala#L65
4.when create HiveClientImpl, it will call SessionState.start(state)

and then in the SessionState.start(state), it will create a default database using hive.metastore.warehouse.dir in hiveConf which is created in HiveClientImpl https://github.com/apache/spark/blob/master/sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala#L189

while the hiveConf created in HiveClientImpl from hadoopConf and sparkConf , and sparkConf will overwrite the value of the same key in hadoopConf. So it means that it actually will use hive.metastore.warehouse.dir in sparkConf to create the default database, if we does not overwrite the value in sparkConf in SharedState, the database location is not we expected which is the warehouse path. So here sparkContext.conf.set("hive.metastore.warehouse.dir", sparkWarehouseDir) should be retained

we can also find that,the default database does not created in SharedState, here condition is false, will not hit the create database logic. it has been created when we init the HiveClientImpl
https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/internal/SharedState.scala#L96

cloud-fan · 2017-02-23T01:56:25Z

sql/core/src/main/scala/org/apache/spark/sql/internal/SharedState.scala

+      logInfo(s"Setting hive.metastore.warehouse.dir ($hiveWarehouseDir) to the value of " +
+        s"${WAREHOUSE_PATH.key} ('$sparkWarehouseDir').")
+      if (hiveWarehouseDir != null) {
+        sparkContext.hadoopConfiguration.set("hive.metastore.warehouse.dir", sparkWarehouseDir)


we can always set it.

yes, maybe when we always set it, the sparkContext.conf.set("hive.metastore.warehouse.dir", sparkWarehouseDir) could be removed, let me test it more, thanks~

SparkQA · 2017-02-23T12:00:31Z

Test build #73340 has finished for PR 16996 at commit 7429cd8.

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

SparkQA · 2017-02-23T12:05:10Z

Test build #73343 has finished for PR 16996 at commit 1d18636.

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

cloud-fan · 2017-02-23T20:19:40Z

sql/core/src/main/scala/org/apache/spark/sql/internal/SharedState.scala

+      logInfo(s"Setting hive.metastore.warehouse.dir ($hiveWarehouseDir) to the value of " +
+        s"${WAREHOUSE_PATH.key} ('$sparkWarehouseDir').")
+      sparkContext.hadoopConfiguration.set("hive.metastore.warehouse.dir", sparkWarehouseDir)
      sparkContext.conf.set("hive.metastore.warehouse.dir", sparkWarehouseDir)


how about sparkContext.conf.remove("hive.metastore.warehouse.dir")? it's good to have a single source of truth.

yes, it is ok to remove it, and it will be more clear that hive.metastore.warehouse.dir and WAREHOUSE_PATH are all at their right places.

cloud-fan · 2017-02-24T03:42:24Z

LGTM

SparkQA · 2017-02-24T04:29:30Z

Test build #73389 has finished for PR 16996 at commit 86deb62.

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

cloud-fan · 2017-02-24T06:58:16Z

thanks, merging to master!

…overwrite its original value ## What changes were proposed in this pull request? In [SPARK-15959](https://issues.apache.org/jira/browse/SPARK-15959), we bring back the `hive.metastore.warehouse.dir` , while in the logic, when use the value of `spark.sql.warehouse.dir` to overwrite `hive.metastore.warehouse.dir` , it set it to `sparkContext.conf` which does not overwrite the value is hadoopConf, I think it should put in `sparkContext.hadoopConfiguration` and overwrite the original value of hadoopConf https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/internal/SharedState.scala#L64 ## How was this patch tested? N/A Author: windpiger <[email protected]> Closes apache#16996 from windpiger/hivemetawarehouseConf.

[SPARK-19664][SQL]put hive.metastore.warehouse.dir in hadoopconf to o…

92c1452

…verwrite its original value

add some loginfo, and fix a bug

ac0a1c6

gatorsmile reviewed Feb 20, 2017

View reviewed changes

windpiger changed the title ~~[SPARK-19664][SQL]put hive.metastore.warehouse.dir in hadoopconf to verwrite its original value~~ [SPARK-19664][SQL]put hive.metastore.warehouse.dir in hadoopconf to overwrite its original value Feb 21, 2017

remove a log info

91b9fd2

cloud-fan reviewed Feb 23, 2017

View reviewed changes

windpiger added 3 commits February 23, 2017 18:03

always set in hadoopconf

7429cd8

modify a comment

26ba212

modify a comment

1d18636

cloud-fan reviewed Feb 23, 2017

View reviewed changes

remove hive.metastore.warehouse.dir in sparkconf

86deb62

windpiger mentioned this pull request Feb 24, 2017

[SPARK-19667][SQL]create table with hiveenabled in default database use warehouse path instead of the location of default database #17001

Closed

asfgit closed this in 8f33731 Feb 24, 2017

[SPARK-19664][SQL]put hive.metastore.warehouse.dir in hadoopconf to overwrite its original value #16996

[SPARK-19664][SQL]put hive.metastore.warehouse.dir in hadoopconf to overwrite its original value #16996

Uh oh!

Conversation

windpiger commented Feb 20, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Feb 20, 2017

Uh oh!

SparkQA commented Feb 20, 2017

Uh oh!

windpiger commented Feb 20, 2017

Uh oh!

SparkQA commented Feb 20, 2017

Uh oh!

windpiger commented Feb 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Feb 20, 2017

Uh oh!

gatorsmile commented Feb 20, 2017

Uh oh!

windpiger commented Feb 21, 2017

Uh oh!

gatorsmile commented Feb 21, 2017

Uh oh!

SparkQA commented Feb 21, 2017

Uh oh!

windpiger commented Feb 22, 2017

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!

windpiger Feb 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

windpiger Feb 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 23, 2017

Uh oh!

SparkQA commented Feb 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Feb 24, 2017

Uh oh!

SparkQA commented Feb 24, 2017

Uh oh!

cloud-fan commented Feb 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

windpiger Feb 23, 2017 •

edited

Loading

windpiger Feb 23, 2017 •

edited

Loading