xkrogen commented on a change in pull request #33101:
URL: https://github.com/apache/spark/pull/33101#discussion_r662452295
##########
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:
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.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
