[SPARK-35907][CORE] Instead of File#mkdirs, Files#createDirectories is expected. by Shockang · Pull Request #33101 · apache/spark

Shockang · 2021-06-26T15:37:08Z

What changes were proposed in this pull request?

The code of method: createDirectory in class: org.apache.spark.util.Utils is modified.

Why are the changes needed?

To solve the problem of ambiguous exception handling in traditional IO creating directories.

What's more, there shouldn't be an improper comment in Spark's source code.

Does this PR introduce any user-facing change?

Yes

The modified method would be called to create the working directory when Worker starts.

The modified method would be called to create local directories for storing block data when the class: DiskBlockManager instantiates.

The modified method would be called to create a temporary directory inside the given parent directory in several classes.

How was this patch tested?

I have provided test cases as much as possible.

Authored-by: Shockang shockang@aliyun.com

…cted.

dongjoon-hyun · 2021-06-26T17:00:59Z

Thank you for making a PR, @Shockang . Could you enable GitHub Action on your Spark fork?

https://spark.apache.org/developer-tools.html (Testing with GitHub Actions workflow)

dongjoon-hyun · 2021-06-26T17:01:36Z

ok to test

…cted.

core/src/main/scala/org/apache/spark/util/Utils.scala

dongjoon-hyun · 2021-06-27T19:24:47Z

core/src/main/scala/org/apache/spark/util/Utils.scala

-          dir = null
-        }
-      } catch { case e: SecurityException => dir = null; }
+        // SPARK-35907 Instead of File#mkdirs, Files#createDirectories is expected.


dongjoon-hyun · 2021-06-27T19:27:40Z

core/src/main/scala/org/apache/spark/util/Utils.scala

-      } catch { case e: SecurityException => dir = null; }
+        // SPARK-35907 Instead of File#mkdirs, Files#createDirectories is expected.
+        Files.createDirectories(dir.toPath)
+      } catch { case e: Exception =>


nit. This could cause a regression because catching Exception can hide lots of things. Is there a way to narrow down this?

nit. This could cause a regression because catching Exception can hide lots of things. Is there a way to narrow down this?

Ignoring the exception directly will cause users to be confused about the creation failure, and Files#createDirectories will throw more meaningful exception information, but SecurityException is not enough to meet the needs.I will take a more appropriate approach.

I meant to use a more specific Exception instead of Ignoring the exception. If there is no other way due to the function signature, it's okay. Thanks for the considering it.

This is a good idea @dongjoon-hyun - Files.createDirectories lists the exceptions which can be thrown, we can restrict to the subset which need to be caught if they are expected to be ignored.

It looks like this conversation was resolved but the issue still persists. I agree with @mridulm, we should just catch IOException and SecurityException here.

I intended to write the code like this:
case e @ (_ : FileAlreadyExistsException | _ : IOException | _ : SecurityException) =>
but considering that the above method: createDirectory(File) also uses Exception, it would be better to keep the same here.And the code is not very concise.

I would argue that createDirectory(File) above is the one doing it wrong and we should correct it there rather than copying it, but will refer to @dongjoon-hyun and @mridulm here.

Btw, FileAlreadyExistsException is an IOException, so you can just do

case e @ (_ : IOException | _ : SecurityException) =>

I would argue that createDirectory(File) above is the one doing it wrong and we should correct it there rather than copying it, but will refer to @dongjoon-hyun and @mridulm here.

Btw, FileAlreadyExistsException is an IOException, so you can just do

case e @ (_ : IOException | _ : SecurityException) =>

I have a little bit of pursuing code simplicity, thanks to your suggestions, the code now looks much more comfortable.o(^▽^)o

dongjoon-hyun · 2021-06-27T19:28:56Z

core/src/main/scala/org/apache/spark/util/Utils.scala

    } catch {
      case e: Exception =>
-        logError(s"Failed to create directory " + dir, e)
+        logError(s"Failed to create directory $dir", e)


This is reasonable. But, in general, I discourage adding irrelevant changes like this. To have a minimal and coherent PR content is the best, @Shockang .

This is reasonable. But, in general, I discourage adding irrelevant changes like this. To have a minimal and coherent PR content is the best, @Shockang .

What you said is right, let me restore it, it makes sense to minimize the modification points.

core/src/main/scala/org/apache/spark/util/Utils.scala

core/src/test/scala/org/apache/spark/util/UtilsSuite.scala

dongjoon-hyun

Thank you for your first contribution, @Shockang . I left a few comments.

Shockang · 2021-06-28T02:13:44Z

Thank you for your first contribution, @Shockang . I left a few comments.

Thanks a lot.I will revise it according to your opinion.

Shockang · 2021-06-29T06:31:47Z

@dongjoon-hyun It has been revised.Could you please verify this patch?

SparkQA · 2021-06-29T20:02:47Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44895/

SparkQA · 2021-06-29T20:57:01Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44895/

SparkQA · 2021-06-29T22:31:15Z

Test build #140364 has finished for PR 33101 at commit 435d125.

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

Ngone51 · 2021-06-30T08:29:22Z

core/src/main/scala/org/apache/spark/util/Utils.scala

+      Files.createDirectories(dir.toPath)
+      true


Hm..this looks like a behavior change to me. Are you sure Files.createDirectories would always not fail silently?

Otherwise, I'd prefer to keep:

if ( !dir.exists() || !dir.isDirectory) { logError(s"Failed to create directory " + dir) } dir.isDirectory

In some extreme scenarios, Files.createDirectories would fail silently. In this case, we can submit an issue to the JDK official. ︿(￣︶￣)︿ The purpose of using this method is to provide better exception handling mechanism, but it should not be at the cost of reducing safety, so I take your suggestion.Thanks a lot. (^▽^)

…cted.

xkrogen · 2021-06-30T15:24:00Z

core/src/main/scala/org/apache/spark/util/Utils.scala

-      } catch { case e: SecurityException => dir = null; }
+        // SPARK-35907 Instead of File#mkdirs, Files#createDirectories is expected.
+        Files.createDirectories(dir.toPath)
+      } catch { case e: Exception =>


It looks like this conversation was resolved but the issue still persists. I agree with @mridulm, we should just catch IOException and SecurityException here.

xkrogen · 2021-06-30T15:29:36Z

core/src/main/scala/org/apache/spark/util/Utils.scala

      if ( !dir.exists() || !dir.isDirectory) {
        logError(s"Failed to create directory " + dir)
      }


@Ngone51 I understand you requested this check to be brought back in your previous comment, but it doesn't seem right to me. Based on the Javadoc for createDirectories, the contract is that the directory will be created successfully, or an exception will be thrown. Keeping the exists/isDirectory checks after is redundant. If we're worried about a JDK bug as described here, why do we trust the implementation of File#exists() and File#isDirectory() but not Files.createDirectories()? IMO the whole point of switching from File to the NIO APIs is that they have more sensible semantics and so we don't need to jump through hoops like this.

Based on the Javadoc for createDirectories, the contract is that the directory will be created successfully, or an exception will be thrown.

I didn't find such a contract there, but only " Unlike the createDirectory method, an exception is not thrown if the directory could not be created because it already exists."...

If we're worried about a JDK bug as described here, why do we trust the implementation of File#exists() and File#isDirectory() but not Files.createDirectories()?

It's not about which method we should trust but which way is safer to go. If there's such a contract as you mentioned, I agree we don't need the check. Otherwise, I think it's safer to follow the original way to avoid suffering from the old issue.

The word "contract" is not used, but I think that is rarely the case in method descriptions. For example none of the methods on Dataset explicitly state contracts or guarantees, but we are expected to assume that the method will do what it says it does, and indicate an error condition using an exception otherwise. My impression here is that the weird semantics of File#mkdirs() are translating into FUD about the potential shortcomings of Files.createDirectories() and that, if the method were taken without that historical context, we wouldn't be worrying about it.

This all being said, it's only three lines of code extra, so of course it is no big detriment to be on the safe side.

The word "contract" is not used, but I think that is rarely the case in method descriptions. For example none of the methods on Dataset explicitly state contracts or guarantees, but we are expected to assume that the method will do what it says it does, and indicate an error condition using an exception otherwise. My impression here is that the weird semantics of File#mkdirs() are translating into FUD about the potential shortcomings of Files.createDirectories() and that, if the method were taken without that historical context, we wouldn't be worrying about it.

This all being said, it's only three lines of code extra, so of course it is no big detriment to be on the safe side.

I agree with you in terms of safety and minimizing modification points.Do you have any questions with my current code?

@Shockang Could you leave a comment above the check to give the historical context, e.g.,

"The check was required by File#mkdirs() because it could sporadically fail silently. After switching to Files.createDirectories(), ideally, there should no longer be silent fails. But the check is kept for the safety concern. We can remove the check when we're sure that Files.createDirectories() would never fail silently."

Do you have any questions with my current code?

No, just wanted to explain my viewpoint. Current code is fine. I agree with @Ngone51 that a comment is helpful to explain the context.

@Shockang Could you leave a comment above the check to give the historical context, e.g.,

"The check was required by File#mkdirs() because it could sporadically fail silently. After switching to Files.createDirectories(), ideally, there should no longer be silent fails. But the check is kept for the safety concern. We can remove the check when we're sure that Files.createDirectories() would never fail silently."

This comment looks perfect. Thank you very much for your comments.

SparkQA · 2021-06-30T15:41:46Z

Test build #140450 has finished for PR 33101 at commit b367ea6.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

…cted.

SparkQA · 2021-06-30T17:21:15Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44964/

SparkQA · 2021-06-30T17:55:58Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44964/

SparkQA · 2021-06-30T18:19:47Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44969/

SparkQA · 2021-06-30T20:12:38Z

Test build #140456 has finished for PR 33101 at commit 983bb36.

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

Ngone51 · 2021-07-02T07:54:11Z

core/src/main/scala/org/apache/spark/util/Utils.scala

+      // SPARK-35907
+      // This could throw more meaningful exception information if directory creation failed.


nit: "SPARK-35907: This could..."

I was going to do that, but the program reported an error: File line length exceeds 100 characters. It's about scalastyle.

I kind of feel the comments aren't necessary? The comment explains why we switched from File#mkdirs() to Files.createDirectories(), but IMO a comment should explain something that is confusing/complicated or unexpected, and if you just come across Files.createDirectories(), there's nothing strange about that.

Ngone51 · 2021-07-02T07:55:17Z

core/src/main/scala/org/apache/spark/util/Utils.scala

+        // SPARK-35907
+        // This could throw more meaningful exception information if directory creation failed.


nit: "SPARK-35907: This could..."

srowen

Looks fine. The tests may be overkill but OK :)
Are there any other instance of .mkdirs() out there that should use the utility method, while we're here?

SparkQA · 2021-07-03T03:23:32Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45120/

SparkQA · 2021-07-03T05:03:55Z

Test build #140608 has finished for PR 33101 at commit a958a5e.

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

SparkQA · 2021-07-03T07:31:56Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45124/

SparkQA · 2021-07-03T08:04:45Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45124/

SparkQA · 2021-07-03T09:16:15Z

Test build #140611 has finished for PR 33101 at commit 6d123ba.

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

Shockang · 2021-07-05T16:07:00Z

Looks fine. The tests may be overkill but OK :)
Are there any other instance of .mkdirs() out there that should use the utility method, while we're here?

Thank you for your attention (｡･∀･)ﾉﾞ In the Spark source code, in addition to the Utils class, there are two user-facing pieces of code and more than 30 test cases using mkdirs. Do you mean I need to replace all these places?

srowen · 2021-07-05T17:32:59Z

I wouldn't worry about the test cases, but if there are places in the source code that look like they probably should leverage the Utils code to make a dir, in order to get all the same behavior, yeah we could change that too here all at once. Up to your judgment - if in doubt post here and we can figure it out.

srowen · 2021-07-07T14:16:20Z

Merged to master

### What changes were proposed in this pull request? Change Worker's workDir from /tmp to Utils.createTempDir in case WorkerSuite ### Why are the changes needed? WorkerSuite fails on MacOS after pr [SPARK-35907](#33101). WorkerSuite tries to create ```/tmp``` dir by ```FIles.createDirectories```, but /tmp is a softlink to /private/tmp on MacOs (both intel and m1), according to suite in [UtilsSuite](https://github.com/apache/spark/blob/master/core/src/test/scala/org/apache/spark/util/UtilsSuite.scala#L525), create directory on existed softlink will cause fail. ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? pass GA Closes #34420 from toujours33/SPARK-37141. Authored-by: Wang <toujours33> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

[SPARK-35907] Instead of File#mkdirs, Files#createDirectories is expe…

d1c2ce1

…cted.

github-actions bot added the CORE label Jun 26, 2021

Merge branch 'master' into SPARK-35907

c66fa90

mridulm approved these changes Jun 26, 2021

View reviewed changes

Shockang added 6 commits June 27, 2021 20:35

scalastyle

71bd12f

scalastyle:File line length exceeds 100 characters

d5ef110

[SPARK-35907] Instead of File#mkdirs, Files#createDirectories is expe…

30b3515

…cted.

[SPARK-35907] Instead of File#mkdirs, Files#createDirectories is expe…

9e3511b

…cted.

Merge branch 'SPARK-35907' of github.com:Shockang/spark into SPARK-35907

000920d

[SPARK-35907] Instead of File#mkdirs, Files#createDirectories is expe…

411f69d

…cted.

dongjoon-hyun reviewed Jun 27, 2021

View reviewed changes

core/src/main/scala/org/apache/spark/util/Utils.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Jun 27, 2021

View reviewed changes

core/src/main/scala/org/apache/spark/util/Utils.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Jun 27, 2021

View reviewed changes

core/src/test/scala/org/apache/spark/util/UtilsSuite.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Jun 27, 2021

View reviewed changes

Optimize comments and exception handling

435d125

Ngone51 reviewed Jun 30, 2021

View reviewed changes

Shockang added 2 commits June 30, 2021 22:54

[SPARK-35907] Instead of File#mkdirs, Files#createDirectories is expe…

1890ca9

…cted.

[SPARK-35907] Instead of File#mkdirs, Files#createDirectories is expe…

b367ea6

…cted.

xkrogen reviewed Jun 30, 2021

View reviewed changes

[SPARK-35907] Instead of File#mkdirs, Files#createDirectories is expe…

983bb36

…cted.

Ngone51 reviewed Jul 2, 2021

View reviewed changes

srowen reviewed Jul 2, 2021

View reviewed changes

Take the comment provided by Ngone51

a958a5e

Take the comment provided by Ngone51

6d123ba

Ngone51 approved these changes Jul 6, 2021

View reviewed changes

srowen closed this in 55373b1 Jul 7, 2021

This was referenced Oct 28, 2021

[SPARK-37147][TESTS] Fix WorkerSuite failure on MacOS #34419

Closed

[SPARK-37141][TESTS] Fix WorkerSuite failure on MacOS #34420

Closed

		// SPARK-35907
		// This could throw more meaningful exception information if directory creation failed.

Conversation

Shockang commented Jun 26, 2021

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!

dongjoon-hyun commented Jun 26, 2021

Uh oh!

dongjoon-hyun commented Jun 26, 2021

Uh oh!

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!

dongjoon-hyun Jun 28, 2021 • 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!

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!

dongjoon-hyun Jun 27, 2021 • 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!

Uh oh!

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

Shockang commented Jun 28, 2021

Uh oh!

Shockang commented Jun 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Jun 29, 2021

Uh oh!

SparkQA commented Jun 29, 2021

Uh oh!

SparkQA commented Jun 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shockang Jun 30, 2021 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 30, 2021

Uh oh!

SparkQA commented Jun 30, 2021

Uh oh!

dongjoon-hyun Jun 28, 2021 •

edited

Loading

dongjoon-hyun Jun 27, 2021 •

edited

Loading

Shockang commented Jun 29, 2021 •

edited

Loading

Shockang Jun 30, 2021 •

edited

Loading

Shockang Jul 2, 2021 •

edited

Loading

xkrogen Jul 2, 2021 •

edited

Loading

Shockang commented Jul 5, 2021 •

edited

Loading