Github user andrewor14 commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2848#discussion_r19990952
  
    --- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
    @@ -412,6 +412,37 @@ private[spark] object Utils extends Logging {
       }
     
       /**
    +   * Download a file from @in to @tempFile, then move it to @destFile, 
checking whether @destFile
    +   * already exists, is equal to the downloaded file, and can be 
overwritten.
    +   */
    +  private def moveFile(
    +      url: String,
    +      in: InputStream,
    +      tempFile: File,
    +      destFile: File,
    +      fileOverwrite: Boolean): Unit = {
    +    val out = new FileOutputStream(tempFile)
    +    Utils.copyStream(in, out, closeStreams = true)
    +    if (destFile.exists && !Files.equal(tempFile, destFile)) {
    +      if (fileOverwrite) {
    +        destFile.delete()
    +        logInfo(("File %s exists and does not match contents of %s, " +
    +          "replacing it with %s").format(destFile, url, url))
    +      } else {
    +        tempFile.delete()
    +        throw new SparkException(
    +          "File " + destFile + " exists and does not match contents of" + 
" " + url)
    +      }
    +    }
    +    // If the destFile exists at this point, it is equal to tempFile and 
we can skip moving
    +    // it; the code above deletes it or throws an Exception, as 
appropriate, if it exists and
    +    // is not equal to tempFile.
    --- End diff --
    
    Hey @ryan-williams my concern is that if the delete fails (e.g. because of 
some permissions thing), then we will always have an old version of the file 
because `destFile.exists` will always be true. It might make sense to throw an 
exception if delete is unsuccessful instead of logging a message up there in 
L429 that is technically not true (I realize you just moved the code, but it 
would still be good to fix this).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to