brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From aledsage <>
Subject [GitHub] incubator-brooklyn pull request: Fix FileBasedObjectStore#moveFile
Date Tue, 15 Sep 2015 10:35:21 GMT
Github user aledsage commented on a diff in the pull request:
    --- Diff: core/src/main/java/org/apache/brooklyn/core/mgmt/persist/
    @@ -346,51 +345,17 @@ protected File backupDirByMoving(File dir) throws InterruptedException,
             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
    -        // 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()) {
        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
and description in,%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 or file a JIRA ticket
with INFRA.

View raw message