Ngone51 commented on a change in pull request #33101:
URL: https://github.com/apache/spark/pull/33101#discussion_r661925134
##########
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:
> 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.
--
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]
