Skip to content

[SPARK-15730][SQL] Respect the --hiveconf in the spark-sql command line - #13542

Closed
chenghao-intel wants to merge 2 commits into
apache:masterfrom
chenghao-intel:hiveconf
Closed

[SPARK-15730][SQL] Respect the --hiveconf in the spark-sql command line#13542
chenghao-intel wants to merge 2 commits into
apache:masterfrom
chenghao-intel:hiveconf

Conversation

@chenghao-intel

@chenghao-intelchenghao-intel commented Jun 7, 2016

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

We should respect the --hiveconf in the spark-sql command line, otherwise, the existing applications based on the spark 1.6 and earlier will broke, as the configurations specified via --hiveconf are missing

How was this patch tested?

I've added the unit test, but still need to be verified with real application.

@SparkQA

SparkQA commented Jun 7, 2016

Copy link
Copy Markdown

Test build #60127 has finished for PR 13542 at commit bbf0ac4.

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

@SparkQA

SparkQA commented Jun 8, 2016

Copy link
Copy Markdown

Test build #60144 has finished for PR 13542 at commit 1fc8a30.

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

@chenghao-intel

chenghao-intel commented Jun 10, 2016

Copy link
Copy Markdown
ContributorAuthor

cc @yhuai

@chenghao-intel

chenghao-intel commented Jun 10, 2016

Copy link
Copy Markdown
ContributorAuthor

Currently, the SparkSQL cli will ignore the configuration passed from commandline via --hiveconf, this will break lots of existing application, it's not by design, isn't it? @yhuai@rxin

We are still verifying if this PR by running the TPCx-BB, will remove the WIP once it passed the test.

yhuai
yhuai reviewed Jun 10, 2016
View reviewed changes
Comment threadsql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/CliSuite.scala
| --hiveconf ${ConfVars.METASTOREWAREHOUSE}=$warehousePath
| --hiveconf ${ConfVars.SCRATCHDIR}=$scratchDirPath
| --hiveconf conf1=conftest
| --hiveconf conf2=1

@yhuaiyhuaiJun 10, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@chenghao-intel Does --hiveconf ${ConfVars.METASTORECONNECTURLKEY}=$jdbcUrl work?

@chenghao-intelchenghao-intelJun 13, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

yes, it works, that's intention, right?

But seems the below code in SparkSQLCliDriver will not work as we expected.

if (key !="javax.jdo.option.ConnectionURL") {
conf.set(key, value)
sessionState.getOverriddenConfigurations.put(key, value)
}

Why do we have to ignore the connection url?

@chenghao-intelchenghao-intelJun 15, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@yhuai any concern for this?

@yhuaiyhuaiJun 16, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you know why those work but something like --hiveconf conf1=conftest does not?

@jameszhouyi

jameszhouyi commented Jun 14, 2016

Copy link
Copy Markdown

Hi @chenghao-intel
This PR is tested and it's OK for my case.

@chenghao-intelchenghao-intel changed the title [SPARK-15730][SQL][WIP] Respect the --hiveconf in the spark-sql command line[SPARK-15730][SQL] Respect the --hiveconf in the spark-sql command lineJun 15, 2016
@chenghao-intel

chenghao-intel commented Jun 15, 2016

Copy link
Copy Markdown
ContributorAuthor

Thanks @jameszhouyi , I've removed the WIP from the title.

@jameszhouyi

jameszhouyi commented Jun 30, 2016

Copy link
Copy Markdown

Hi Spark community ，
Could you please help to review this PR to merge in 2.0.0 ? Since this bug has broken our real cases. Thanks in advance !

@yhuai

yhuai commented Jun 30, 2016

Copy link
Copy Markdown
Contributor

Can you provide more information on the root cause? Seems it is not clear why it does not work.

@yhuai

yhuai commented Jul 1, 2016

Copy link
Copy Markdown
Contributor

also, can you try --conf and see if it works?

@chenghao-intel

chenghao-intel commented Jul 1, 2016

Copy link
Copy Markdown
ContributorAuthor

@yhuai I couldn't find any piece of code to copy the HiveConf(from SessionState) to SqlConf? Can you confirm this?

Probably that's the reason why --hiveconf doesn't work.

@jameszhouyi

jameszhouyi commented Jul 5, 2016

Copy link
Copy Markdown

Hi,
Could you please help to review this PR to merge it in 2.0.0 ? this broken thing blocked our testing. Thanks!

@yhuaiyhuai mentioned this pull request Jul 5, 2016
Closed
@yhuai

yhuai commented Jul 5, 2016

Copy link
Copy Markdown
Contributor

I have opened https://github.com/apache/spark/pull/14058/files (it has one update).

asfgit pushed a commit that referenced this pull request Jul 5, 2016
@chenghao-intel@rxin
[SPARK-15730][SQL] Respect the --hiveconf in the spark-sql command line
9c1596b
## What changes were proposed in this pull request?
This PR makes spark-sql (backed by SparkSQLCLIDriver) respects confs set by hiveconf, which is what we do in previous versions. The change is that when we start SparkSQLCLIDriver, we explicitly set confs set through --hiveconf to SQLContext's conf (basically treating those confs as a SparkSQL conf).
## How was this patch tested?
A new test in CliSuite.
Closes#13542
Author: Cheng Hao <hao.cheng@intel.com>
Author: Yin Huai <yhuai@databricks.com>
Closes#14058 from yhuai/hiveConfThriftServer.
(cherry picked from commit 920cb5f)
Signed-off-by: Reynold Xin <rxin@databricks.com>
@asfgitasfgit closed this in 920cb5fJul 5, 2016
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@chenghao-intel@SparkQA@jameszhouyi@yhuai