Skip to content

[SPARK-23514] Use SessionState.newHadoopConf() to propage hadoop configs set in SQLConf. #20679

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
juliuszsompolski wants to merge 2 commits into from
Closed

[SPARK-23514] Use SessionState.newHadoopConf() to propage hadoop configs set in SQLConf. #20679

juliuszsompolski wants to merge 2 commits into from

Conversation

@juliuszsompolski
Copy link
Contributor

@juliuszsompolski juliuszsompolski commented Feb 26, 2018

What changes were proposed in this pull request?

A few places in spark-sql were using sc.hadoopConfiguration directly. They should be using sessionState.newHadoopConf() to blend in configs that were set through SQLConf.

Also, for better UX, for these configs blended in from SQLConf, we should consider removing the spark.hadoop prefix, so that the settings are recognized whether or not they were specified by the user.

How was this patch tested?

Tested that AlterTableRecoverPartitions now correctly recognizes settings that are passed in to the FileSystem through SQLConf.

@juliuszsompolski
Copy link
Contributor Author

juliuszsompolski commented Feb 26, 2018

cc @gatorsmile @rxin
We had a chat whether to implement something that would catch direct misuses of sc.hadoopConfiguration in sql module, but it seems that it's not very common, so maybe just fixing it where it happened is enough.

@liancheng suggested stripping the "spark.hadoop" prefix to have more compatibility with users specifying or not specifying that prefix.

@juliuszsompolski
Copy link
Contributor Author

juliuszsompolski commented Feb 26, 2018

cc @dongjoon-hyun

gatorsmile
gatorsmile reviewed Feb 26, 2018
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/internal/SessionState.scala Outdated
val newHadoopConf = new Configuration(hadoopConf)
sqlConf.getAllConfs.foreach { case (k, v) => if (v ne null) newHadoopConf.set(k, v) }
sqlConf.getAllConfs.foreach { case (k, v) =>
if (v ne null) newHadoopConf.set(k, v.stripPrefix("spark.hadoop"))
Copy link
Member

@gatorsmile gatorsmile Feb 26, 2018

Choose a reason for hiding this comment

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

Could we add a test case for this feature?

Copy link
Contributor Author

@juliuszsompolski juliuszsompolski Feb 27, 2018

Choose a reason for hiding this comment

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

I just reverted this part, it's not really related to the rest.

@SparkQA
Copy link

SparkQA commented Feb 26, 2018

Test build #87675 has finished for PR 20679 at commit 2c070fc.

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

@SparkQA
Copy link

SparkQA commented Feb 27, 2018

Test build #87730 has finished for PR 20679 at commit b37f24f.

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

@juliuszsompolski
Copy link
Contributor Author

juliuszsompolski commented Feb 27, 2018

jenkins retest this please
Flaky: sbt.ForkMain$ForkError: java.io.IOException: Failed to delete: /home/jenkins/workspace/SparkPullRequestBuilder/target/tmp/spark-c0f7c3f9-f48a-4bb8-91f7-e9c710e78b00

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Feb 27, 2018

Retest this please.

@SparkQA
Copy link

SparkQA commented Feb 27, 2018

Test build #87733 has finished for PR 20679 at commit b37f24f.

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

@gatorsmile
Copy link
Member

gatorsmile commented Feb 28, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Feb 28, 2018

Test build #87738 has finished for PR 20679 at commit b37f24f.

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

@gatorsmile
Copy link
Member

gatorsmile commented Feb 28, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Feb 28, 2018

Test build #87753 has finished for PR 20679 at commit b37f24f.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@gatorsmile
Copy link
Member

gatorsmile commented Feb 28, 2018

retest this please

@juliuszsompolski
Copy link
Contributor Author

juliuszsompolski commented Feb 28, 2018

jenkins retest this please

@SparkQA
Copy link

SparkQA commented Feb 28, 2018

Test build #87774 has finished for PR 20679 at commit b37f24f.

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

gatorsmile
gatorsmile reviewed Feb 28, 2018
View reviewed changes
Copy link
Member

@gatorsmile gatorsmile left a comment

Choose a reason for hiding this comment

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

LGTM

@gatorsmile
Copy link
Member

gatorsmile commented Feb 28, 2018

Thanks! Merged to master.

@asfgit asfgit closed this in 476a7f0 Feb 28, 2018
@juliuszsompolski juliuszsompolski mentioned this pull request Mar 2, 2018
Closed
asfgit pushed a commit that referenced this pull request Mar 3, 2018
@juliuszsompolski @cloud-fan
[SPARK-23514][FOLLOW-UP] Remove more places using sparkContext.hadoop…
dea381d 
…Configuration directly

## What changes were proposed in this pull request?

In #20679 I missed a few places in SQL tests.
For hygiene, they should also use the sessionState interface where possible.

## How was this patch tested?

Modified existing tests.

Author: Juliusz Sompolski <[email protected]>

Closes #20718 from juliuszsompolski/SPARK-23514-followup.
zzcclp added a commit to zzcclp/spark that referenced this pull request Dec 6, 2018
@zzcclp
[EXT][SPARK-23514] Use SessionState.newHadoopConf() to propage hadoop…
9558b9f 
… configs set in SQLConf. apache#20679

A few places in spark-sql were using sc.hadoopConfiguration directly. They should be using sessionState.newHadoopConf() to blend in configs that were set through SQLConf.

Also, for better UX, for these configs blended in from SQLConf, we should consider removing the spark.hadoop prefix, so that the settings are recognized whether or not they were specified by the user.
zzcclp added a commit to zzcclp/spark that referenced this pull request Sep 20, 2019
@zzcclp
[EXT][SPARK-23514] Use SessionState.newHadoopConf() to propage hadoop…
667abf1 
… configs set in SQLConf. apache#20679

A few places in spark-sql were using sc.hadoopConfiguration directly. They should be using sessionState.newHadoopConf() to blend in configs that were set through SQLConf.

Also, for better UX, for these configs blended in from SQLConf, we should consider removing the spark.hadoop prefix, so that the settings are recognized whether or not they were specified by the user.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gatorsmile gatorsmile gatorsmile left review comments

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

@juliuszsompolski @SparkQA @dongjoon-hyun @gatorsmile