xkrogen commented on a change in pull request #33101:
URL: https://github.com/apache/spark/pull/33101#discussion_r661581188
##########
File path: core/src/main/scala/org/apache/spark/util/Utils.scala
##########
@@ -310,15 +306,17 @@ private[spark] object Utils extends Logging {
while (dir == null) {
attempts += 1
if (attempts > maxAttempts) {
- throw new IOException("Failed to create a temp directory (under " +
root + ") after " +
- maxAttempts + " attempts!")
+ throw new IOException(s"Failed to create a temp directory (under
$root) after " +
+ s"$maxAttempts attempts!")
}
try {
dir = new File(root, namePrefix + "-" + UUID.randomUUID.toString)
- if (dir.exists() || !dir.mkdirs()) {
- dir = null
- }
- } catch { case e: SecurityException => dir = null; }
+ // SPARK-35907 Instead of File#mkdirs, Files#createDirectories is
expected.
+ Files.createDirectories(dir.toPath)
+ } catch { case e: Exception =>
Review comment:
It looks like this conversation was resolved but the issue still
persists. I agree with @mridulm, we should just catch `IOException` and
`SecurityException` here.
##########
File path: core/src/main/scala/org/apache/spark/util/Utils.scala
##########
@@ -285,9 +285,10 @@ private[spark] object Utils extends Logging {
*/
def createDirectory(dir: File): Boolean = {
try {
- // This sporadically fails - not sure why ... !dir.exists() &&
!dir.mkdirs()
- // So attempting to create and then check if directory was created or
not.
- dir.mkdirs()
+ // SPARK-35907
+ // This could throw more meaningful exception information if directory
creation failed.
+ // To be on the safe side, try to create and then check if directory was
created or not.
+ Files.createDirectories(dir.toPath)
if ( !dir.exists() || !dir.isDirectory) {
logError(s"Failed to create directory " + dir)
}
Review comment:
@Ngone51 I understand you requested this check to be brought back in
your [previous
comment](https://github.com/apache/spark/pull/33101#discussion_r661245031), but
it doesn't seem right to me. Based on the [Javadoc for
createDirectories](https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#createDirectories(java.nio.file.Path,%20java.nio.file.attribute.FileAttribute...)),
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](https://github.com/apache/spark/pull/33101#discussion_r661579528), 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
