Mailing-List: contact dev-help@brooklyn.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@brooklyn.incubator.apache.org
From: aledsage <git@git.apache.org>
To: dev@brooklyn.incubator.apache.org
Reply-To: dev@brooklyn.incubator.apache.org
References: <git-pr-891-incubator-brooklyn@git.apache.org>
In-Reply-To: <git-pr-891-incubator-brooklyn@git.apache.org>
Subject: [GitHub] incubator-brooklyn pull request: Fix
 FileBasedObjectStore#moveFile
Content-Type: text/plain
Message-Id: <20150915103521.4FC4EDFE7C@git1-us-west.apache.org>
Date: Tue, 15 Sep 2015 10:35:21 +0000 (UTC)

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

    https://github.com/apache/incubator-brooklyn/pull/891#discussion_r39495889
  
    --- Diff: core/src/main/java/org/apache/brooklyn/core/mgmt/persist/FileBasedObjectStore.java ---
    @@ -346,51 +345,17 @@ protected File backupDirByMoving(File dir) throws InterruptedException, IOExcept
             return newDir;
         }
     
    -    private static boolean WARNED_ON_NON_ATOMIC_FILE_UPDATES = false; 
         /** 
          * Attempts an fs level atomic move then fall back to pure java rename.
          * Assumes files are on same mount point.
    -     * <p>
    -     * TODO Java 7 gives an atomic Files.move() which would be preferred.
    +     * Overwriting existing destFile
          */
         static void moveFile(File srcFile, File destFile) throws IOException, InterruptedException {
    -        // Try rename first - it is a *much* cheaper call than invoking a system call in Java. 
    -        // However, rename is not guaranteed cross platform to succeed if the destination exists,
    -        // and not guaranteed to be atomic, but it usually seems to do the right thing...
    -        boolean result;
    -        result = srcFile.renameTo(destFile);
    -        if (result) {
    -            if (log.isTraceEnabled()) log.trace("java rename of {} to {} completed", srcFile, destFile);
    -            return;
    -        }
    -        
    -        if (!Os.isMicrosoftWindows()) {
    -            // this command, if it succeeds, is guaranteed to be atomic, and it will usually overwrite
    -            String cmd = "mv '"+srcFile.getAbsolutePath()+"' '"+destFile.getAbsolutePath()+"'";
    -            
    -            int exitStatus = new ProcessTool().execCommands(MutableMap.<String,String>of(), MutableList.of(cmd), null);
    -            // prefer the above to the below because it wraps it in the appropriate bash
    -//            Process proc = Runtime.getRuntime().exec(cmd);
    -//            result = proc.waitFor();
    -            
    -            if (log.isTraceEnabled()) log.trace("FS move of {} to {} completed, code {}", new Object[] { srcFile, destFile, exitStatus });
    -            if (exitStatus == 0) return;
    +        if (destFile.exists()) {
    --- End diff --
    
    Instead of doing the delete followed by the move, can we first try (this will work on some filesystems, replacing the existing file atomically):
    
        Files.move(srcFile.toPath(), destFile.toPath(), StandardCopyOption.ATOMIC_MOVE);
    
    If that fails (with an `AtomicMoveNotSupportedException`), then we can fallback to other options. We'll want to include the log.warn in this situation. Not sure how complicated we want to make it! I'm not sure how much the exception would destroy our performance if it's happening one or two times on every attempted file move.
    
    Could try deleting and then atomically moving:
    
        if (destFile.exists()) {
            destFile.delete();
        }
        Files.move(srcFile.toPath(), destFile.toPath(), StandardCopyOption.ATOMIC_MOVE);
    
    And could try a non-atomic move:
    
        Files.move(srcFile.toPath(), destFile.toPath(), StandardCopyOption.REPLACE_EXISTING);
    
    See discussion in http://stackoverflow.com/questions/595631/how-to-atomically-rename-a-file-in-java-even-if-the-dest-file-already-exists and description in http://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#move(java.nio.file.Path,%20java.nio.file.Path,%20java.nio.file.CopyOption...)


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---