Skip to content

Conversation

@gengliangwang
Copy link
Member

What changes were proposed in this pull request?

In most cases, we should use spark.sessionState.newHadoopConf() instead of sparkContext.hadoopConfiguration, so that the hadoop configurations specified in Spark session
configuration will come into effect.

Add a rule matching spark.sparkContext.hadoopConfiguration or spark.sqlContext.sparkContext.hadoopConfiguration to prevent the usage.

How was this patch tested?

Unit test

</check>

<check customId="hadoopconfiguration" level="error" class="org.scalastyle.file.RegexChecker" enabled="true">
<parameters><parameter name="regex">spark(.sqlContext)?.sparkContext.hadoopConfiguration</parameter></parameters>
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just match on hadoopConfiguration ?

Copy link
Member Author

@gengliangwang gengliangwang Jul 26, 2018

Choose a reason for hiding this comment

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

I think that would be tricky:

  1. In core module the function spark.sessionState.newHadoopConf() can't be accessed.
  2. Run find . -name *.scala|xargs grep hadoopConfiguration |wc -l and there are 100 appearances.

Copy link
Member Author

Choose a reason for hiding this comment

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

Even with such loose policy, we have to wrap code block with "// scalastyle:off hadoopconfiguration" in some test cases for setting the hadoop conf.

@SparkQA
Copy link

SparkQA commented Jul 25, 2018

Test build #93552 has finished for PR 21873 at commit 8b5ad70.

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

@SparkQA
Copy link

SparkQA commented Jul 26, 2018

Test build #93592 has finished for PR 21873 at commit a20206d.

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

@gatorsmile
Copy link
Member

Thanks! Merged to master

@asfgit asfgit closed this in fa09d91 Jul 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants