[SPARK-32539][INFRA] Disallow `FileSystem.get(Configuration conf)` in style check by default #29357

gengliangwang · 2020-08-05T07:58:42Z

What changes were proposed in this pull request?

Disallow FileSystem.get(Configuration conf) in Scala style check by default and suggest developers use FileSystem.get(URI uri, Configuration conf) or Path.getFileSystem() instead.

Why are the changes needed?

The method FileSystem.get(Configuration conf) will return a default FileSystem instance if the conf fs.file.impl is not set. This can cause file not found exception on reading a target path of non-default file system, e.g. S3. It is hard to discover such a mistake via unit tests.
If we disallow it in Scala style check by default and suggest developers use FileSystem.get(URI uri, Configuration conf) or Path.getFileSystem(Configuration conf), we can reduce potential regression and PR review effort.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Manually run scala style check and test.

SparkQA · 2020-08-05T10:25:54Z

Test build #127089 has finished for PR 29357 at commit 06e8f8f.

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

HyukjinKwon · 2020-08-05T11:11:23Z

core/src/main/scala/org/apache/spark/deploy/security/HadoopFSDelegationTokenProvider.scala

      sparkConf: SparkConf,
      hadoopConf: Configuration): Set[FileSystem] = {
+    // scalastyle:off FileSystemGet
    val defaultFS = FileSystem.get(hadoopConf)


I think it's a bit arguable to disallow it in style. If you want to just get the default file system (when path is not available), FileSystem.get is a valid usage and a good fallback to use.

I agree this is arguable. The rule is motivated by a regression of another project. I think it is worth doing so if we treat it as a warning for potential mistakes.

Okay, I am fine with it.

cloud-fan · 2020-08-05T14:46:23Z

scalastyle-config.xml

+    <customMessage><![CDATA[
+      Are you sure that you want to use "FileSystem.get(Configuration conf)"? If the input
+      configuration is not set properly, a default FileSystem instance will be returned. Please use
+      "FileSystem.get(URI uri, Configuration conf)" or "Path.getFileSystem()" instead.


Path.getFileSystem() also takes a conf parameter?

Right, thank you.

cloud-fan · 2020-08-05T14:49:59Z

@gengliangwang can you put a real example in the PR description? e.g. Path#getFileSystem will respect the default file scheme of the path and create a file system for s3, while FileSystem.get(Configuration conf) may return a file system for local files.

gengliangwang · 2020-08-05T15:12:03Z

@gengliangwang can you put a real example in the PR description? e.g. Path#getFileSystem will respect the default file scheme of the path and create a file system for s3, while FileSystem.get(Configuration conf) may return a file system for local files.

Thanks, I have updated the description.

xkrogen

I worked on Hadoop (as a committer on HDFS) for a few years before starting my work on Spark, and I can attest that this is a common anti-pattern that can have severe implications. Even within the Hadoop codebase itself, I've had to go through and rip out old code which leveraged FileSystem.get(Configuration conf) because it breaks under some environments. Many developers don't realize it is almost always not the correct API to use.

So, strong +1 from me on this effort.

xkrogen · 2020-08-05T15:28:43Z

scalastyle-config.xml

+      Are you sure that you want to use "FileSystem.get(Configuration conf)"? If the input
+      configuration is not set properly, a default FileSystem instance will be returned. Please use
+      "FileSystem.get(URI uri, Configuration conf)" or "Path.getFileSystem(Configuration conf)" instead.


Using FileSystem.get(Configuration conf) is dangerous even outside of the misconfiguration scenario you describe. It's entirely possible for Spark applications to be working with multiple FileSystem instances (say, local HDFS and a cloud blob store, or two HDFS clusters). Using FileSystem.get(Configuration conf) in this instance can also result in nasty errors.

Not sure if it's worth mentioning here, will leave that up to your judgement.

@xkrogen Thanks for the +1! I just update the message in style check. I think mentioning "reading a target path of non-default file system" is good enough.

how about

Are you sure that you want to use "FileSystem.get(Configuration conf)"? If the input configuration is not set properly, a default FileSystem instance will be returned. It can also lead to nasty errors when you deal with multiple file systems. Please use ....

@cloud-fan ok, updated.

srowen · 2020-08-05T15:35:18Z

Seems reasonable to guard against. I know we have fixed this a few times over the years.

SparkQA · 2020-08-05T17:02:07Z

Test build #127097 has finished for PR 29357 at commit edb1114.

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

dongjoon-hyun · 2020-08-05T17:43:10Z

.../scala/org/apache/spark/sql/execution/datasources/parquet/ParquetInteroperabilitySuite.scala


              val hadoopConf = spark.sessionState.newHadoopConf()
-              val fs = FileSystem.get(hadoopConf)
+              val fs = new Path(tableDir.getAbsolutePath).getFileSystem(hadoopConf)


Can we have this pattern at https://github.com/apache/spark/pull/29357/files#diff-2f2d1e1ffed122e56f3cb20cf7c320d6R276, too?

@dongjoon-hyun Do you mean making it

// scalastyle:off FileSystemGet val fs = FileSystem.get(hadoopConf) // scalastyle:on FileSystemGet

here?

The method Path.getFileSystem(Configuration) is suggested: https://github.com/apache/spark/pull/29357/files#diff-2f2d1e1ffed122e56f3cb20cf7c320d6R273

Ideally we should not change the actual code when we just want to add a style check. But this is test and I think it's safe.

You are right, @gengliangwang ~

SparkQA · 2020-08-05T18:46:30Z

Test build #127098 has finished for PR 29357 at commit b08aee6.

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

SparkQA · 2020-08-05T19:07:59Z

Test build #127100 has finished for PR 29357 at commit 92f3d6f.

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

cloud-fan · 2020-08-06T05:56:57Z

thanks, merging to master!

dongjoon-hyun · 2020-08-06T14:36:33Z

+1, late LGTM.

disallow FileSystem.get(conf)

06e8f8f

probot-autolabeler bot added BUILD CORE SQL YARN labels Aug 5, 2020

gengliangwang requested a review from cloud-fan August 5, 2020 07:59

HyukjinKwon reviewed Aug 5, 2020

View reviewed changes

cloud-fan reviewed Aug 5, 2020

View reviewed changes

address comment

edb1114

xkrogen reviewed Aug 5, 2020

View reviewed changes

gengliangwang added 2 commits August 6, 2020 00:05

improve message

b08aee6

update message

92f3d6f

dongjoon-hyun reviewed Aug 5, 2020

View reviewed changes

cloud-fan closed this in e93b8f0 Aug 6, 2020

[SPARK-32539][INFRA] Disallow FileSystem.get(Configuration conf) in style check by default #29357

[SPARK-32539][INFRA] Disallow FileSystem.get(Configuration conf) in style check by default #29357

Uh oh!

Conversation

gengliangwang commented Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

SparkQA commented Aug 5, 2020

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!

cloud-fan commented Aug 5, 2020

Uh oh!

gengliangwang commented Aug 5, 2020

Uh oh!

xkrogen left a comment

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!

srowen commented Aug 5, 2020

Uh oh!

SparkQA commented Aug 5, 2020

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!

SparkQA commented Aug 5, 2020

Uh oh!

SparkQA commented Aug 5, 2020

Uh oh!

cloud-fan commented Aug 6, 2020

Uh oh!

dongjoon-hyun commented Aug 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

[SPARK-32539][INFRA] Disallow `FileSystem.get(Configuration conf)` in style check by default #29357

[SPARK-32539][INFRA] Disallow `FileSystem.get(Configuration conf)` in style check by default #29357

gengliangwang commented Aug 5, 2020 •

edited

Loading