Uploaded image for project: 'Flink'
  1. Flink
  2. FLINK-6103

LocalFileSystem rename() uses File.renameTo()

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.3.0
    • Component/s: Local Runtime
    • Labels:
    • Flags:
      Important

      Description

      I've tried to move a directory to another on the LocalFilesystem and it doesn't work (in my case fs is an instance of java.io.UnixFileSystem).
      As for Flink-1840 (there was a PR to fix the issue - https://github.com/apache/flink/pull/578) the problem is that File.renameTo() is not reliable.

      Indeed, the Javadoc says:

      Renames the file denoted by this abstract pathname. Many aspects of the behavior of this method are inherently platform-dependent: The rename operation might not be able to move a file from one filesystem to another, it might not be atomic, and it might not succeed if a file with the destination abstract pathname already exists. The return value should always be checked to make sure that the rename operation was successful. Note that the java.nio.file.Files class defines the move method to move or rename a file in a platform independent manner

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/flink/pull/3598

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/3598
          Hide
          StephanEwen Stephan Ewen added a comment -

          Fixed via a4f9a54953132fbd1a44888a403576c157649f1f and f9eac3afd4c613df8bc577c080e71230d98df87a

          Show
          StephanEwen Stephan Ewen added a comment - Fixed via a4f9a54953132fbd1a44888a403576c157649f1f and f9eac3afd4c613df8bc577c080e71230d98df87a
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on the issue:

          https://github.com/apache/flink/pull/3598

          Thanks, I'll take it from here...

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3598 Thanks, I'll take it from here...
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fpompermaier commented on the issue:

          https://github.com/apache/flink/pull/3598

          Now it should be ok

          Show
          githubbot ASF GitHub Bot added a comment - Github user fpompermaier commented on the issue: https://github.com/apache/flink/pull/3598 Now it should be ok
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on the issue:

          https://github.com/apache/flink/pull/3598

          I would do the following:

          • log nothing
          • Catch the errors that are regular "move failed" exceptions and return false.
          • `FileNotFoundException`
          • `DirectoryNotEmptyException`
          • `SecurityException`
          • Let all other `IOExceptions` bubble out
          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3598 I would do the following: log nothing Catch the errors that are regular "move failed" exceptions and return false. `FileNotFoundException` `DirectoryNotEmptyException` `SecurityException` Let all other `IOExceptions` bubble out
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fpompermaier commented on the issue:

          https://github.com/apache/flink/pull/3598

          Of course..but how should I handle them? should I catch just one exception? What should I log?

          Show
          githubbot ASF GitHub Bot added a comment - Github user fpompermaier commented on the issue: https://github.com/apache/flink/pull/3598 Of course..but how should I handle them? should I catch just one exception? What should I log?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zentol commented on the issue:

          https://github.com/apache/flink/pull/3598

          @fpompermaier have you addressed the comment @StephanEwen regarding logging?

          Show
          githubbot ASF GitHub Bot added a comment - Github user zentol commented on the issue: https://github.com/apache/flink/pull/3598 @fpompermaier have you addressed the comment @StephanEwen regarding logging?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fpompermaier commented on the issue:

          https://github.com/apache/flink/pull/3598

          Anyone willing to merge this PR...?

          Show
          githubbot ASF GitHub Bot added a comment - Github user fpompermaier commented on the issue: https://github.com/apache/flink/pull/3598 Anyone willing to merge this PR...?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on the issue:

          https://github.com/apache/flink/pull/3598

          I think we don't need the logging in all of the cases. We typically avoid logging in "utility functions", which are missing the context if the exception is in fact a problem worth mentioning, of if it only adds noise to the log.

          Other than that, I think its good.

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3598 I think we don't need the logging in all of the cases. We typically avoid logging in "utility functions", which are missing the context if the exception is in fact a problem worth mentioning, of if it only adds noise to the log. Other than that, I think its good.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fpompermaier commented on the issue:

          https://github.com/apache/flink/pull/3598

          Does the current exception handling address your comments @StephanEwen?

          Show
          githubbot ASF GitHub Bot added a comment - Github user fpompermaier commented on the issue: https://github.com/apache/flink/pull/3598 Does the current exception handling address your comments @StephanEwen?
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/flink/pull/3598#discussion_r107925602

          — Diff: flink-core/src/main/java/org/apache/flink/core/fs/local/LocalFileSystem.java —
          @@ -262,8 +265,13 @@ public FSDataOutputStream create(
          public boolean rename(final Path src, final Path dst) throws IOException {
          final File srcFile = pathToFile(src);
          final File dstFile = pathToFile(dst);
          -

          • return srcFile.renameTo(dstFile);
            + //Files.move fails if the destination directory doesn't exist
            + if(dstFile.getParentFile()!= null && !dstFile.getParentFile().exists()) { + Files.createDirectories(dstFile.getParentFile().toPath()); + }

            + //this throws an IOException if any error occurs
            + Files.move(srcFile.toPath(), dstFile.toPath(), StandardCopyOption.REPLACE_EXISTING);

              • End diff –

          I think that is inherited from the interface that is also used by the HdfsFileSystem. The outcomes are `true`(rename success), `false` (rename failed), and `IOExeption` (communication with filesystem failed).

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3598#discussion_r107925602 — Diff: flink-core/src/main/java/org/apache/flink/core/fs/local/LocalFileSystem.java — @@ -262,8 +265,13 @@ public FSDataOutputStream create( public boolean rename(final Path src, final Path dst) throws IOException { final File srcFile = pathToFile(src); final File dstFile = pathToFile(dst); - return srcFile.renameTo(dstFile); + //Files.move fails if the destination directory doesn't exist + if(dstFile.getParentFile()!= null && !dstFile.getParentFile().exists()) { + Files.createDirectories(dstFile.getParentFile().toPath()); + } + //this throws an IOException if any error occurs + Files.move(srcFile.toPath(), dstFile.toPath(), StandardCopyOption.REPLACE_EXISTING); End diff – I think that is inherited from the interface that is also used by the HdfsFileSystem. The outcomes are `true`(rename success), `false` (rename failed), and `IOExeption` (communication with filesystem failed).
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/flink/pull/3598#discussion_r107894608

          — Diff: flink-core/src/main/java/org/apache/flink/core/fs/local/LocalFileSystem.java —
          @@ -262,8 +265,13 @@ public FSDataOutputStream create(
          public boolean rename(final Path src, final Path dst) throws IOException {
          final File srcFile = pathToFile(src);
          final File dstFile = pathToFile(dst);
          -

          • return srcFile.renameTo(dstFile);
            + //Files.move fails if the destination directory doesn't exist
            + if(dstFile.getParentFile()!= null && !dstFile.getParentFile().exists()) { + Files.createDirectories(dstFile.getParentFile().toPath()); + }

            + //this throws an IOException if any error occurs
            + Files.move(srcFile.toPath(), dstFile.toPath(), StandardCopyOption.REPLACE_EXISTING);

              • End diff –

          So why is the method trowing an IOException?

          Show
          githubbot ASF GitHub Bot added a comment - Github user fpompermaier commented on a diff in the pull request: https://github.com/apache/flink/pull/3598#discussion_r107894608 — Diff: flink-core/src/main/java/org/apache/flink/core/fs/local/LocalFileSystem.java — @@ -262,8 +265,13 @@ public FSDataOutputStream create( public boolean rename(final Path src, final Path dst) throws IOException { final File srcFile = pathToFile(src); final File dstFile = pathToFile(dst); - return srcFile.renameTo(dstFile); + //Files.move fails if the destination directory doesn't exist + if(dstFile.getParentFile()!= null && !dstFile.getParentFile().exists()) { + Files.createDirectories(dstFile.getParentFile().toPath()); + } + //this throws an IOException if any error occurs + Files.move(srcFile.toPath(), dstFile.toPath(), StandardCopyOption.REPLACE_EXISTING); End diff – So why is the method trowing an IOException?
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/flink/pull/3598#discussion_r107863788

          — Diff: flink-core/src/main/java/org/apache/flink/core/fs/local/LocalFileSystem.java —
          @@ -262,8 +265,13 @@ public FSDataOutputStream create(
          public boolean rename(final Path src, final Path dst) throws IOException {
          final File srcFile = pathToFile(src);
          final File dstFile = pathToFile(dst);
          -

          • return srcFile.renameTo(dstFile);
            + //Files.move fails if the destination directory doesn't exist
            + if(dstFile.getParentFile()!= null && !dstFile.getParentFile().exists()) { + Files.createDirectories(dstFile.getParentFile().toPath()); + }

            + //this throws an IOException if any error occurs
            + Files.move(srcFile.toPath(), dstFile.toPath(), StandardCopyOption.REPLACE_EXISTING);

              • End diff –

          If we want to preserve the old semantics, we should catch the exception and return `false` in that case.

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3598#discussion_r107863788 — Diff: flink-core/src/main/java/org/apache/flink/core/fs/local/LocalFileSystem.java — @@ -262,8 +265,13 @@ public FSDataOutputStream create( public boolean rename(final Path src, final Path dst) throws IOException { final File srcFile = pathToFile(src); final File dstFile = pathToFile(dst); - return srcFile.renameTo(dstFile); + //Files.move fails if the destination directory doesn't exist + if(dstFile.getParentFile()!= null && !dstFile.getParentFile().exists()) { + Files.createDirectories(dstFile.getParentFile().toPath()); + } + //this throws an IOException if any error occurs + Files.move(srcFile.toPath(), dstFile.toPath(), StandardCopyOption.REPLACE_EXISTING); End diff – If we want to preserve the old semantics, we should catch the exception and return `false` in that case.
          Hide
          f.pompermaier Flavio Pompermaier added a comment -
          Show
          f.pompermaier Flavio Pompermaier added a comment - Here's the PR link: https://github.com/apache/flink/pull/3598
          Hide
          f.pompermaier Flavio Pompermaier added a comment -

          Maybe also BlobServerConnection could benefit from the same fix (in order to avoid to rely on Guava)

          Show
          f.pompermaier Flavio Pompermaier added a comment - Maybe also BlobServerConnection could benefit from the same fix (in order to avoid to rely on Guava)
          Hide
          StephanEwen Stephan Ewen added a comment -

          +1 to go with {{Files.move(source, target, REPLACE_EXISTING)}

          Show
          StephanEwen Stephan Ewen added a comment - +1 to go with {{Files.move(source, target, REPLACE_EXISTING)}
          Hide
          f.pompermaier Flavio Pompermaier added a comment -

          I've tried com.google.common.io.Files.move() and it doesn't work either with directories.

          Maybe it's better to follow https://docs.oracle.com/javase/tutorial/essential/io/move.html and use Files.move(source, target, REPLACE_EXISTING) ?

          Show
          f.pompermaier Flavio Pompermaier added a comment - I've tried com.google.common.io.Files.move() and it doesn't work either with directories. Maybe it's better to follow https://docs.oracle.com/javase/tutorial/essential/io/move.html and use Files.move(source, target, REPLACE_EXISTING) ?

            People

            • Assignee:
              f.pompermaier Flavio Pompermaier
              Reporter:
              f.pompermaier Flavio Pompermaier
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development