[GitHub] [incubator-gobblin] sv2000 commented on a change in pull request #2986: [GOBBLIN-1147]Use one dfsClient in FsDataWriter to to rename and exists check to av…

[GitBox <gi...@apache.org>]

sv2000 commented on a change in pull request #2986:
URL: https://github.com/apache/incubator-gobblin/pull/2986#discussion_r425568355



##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/HadoopUtils.java
##########
@@ -266,19 +268,25 @@ public static void renamePath(FileSystem fs, Path oldName, Path newName) throws
    * {@link FileSystem#rename(Path, Path)} returns False.
    */
   public static void renamePath(FileSystem fs, Path oldName, Path newName, boolean overwrite) throws IOException {
-    if (!fs.exists(oldName)) {
-      throw new FileNotFoundException(String.format("Failed to rename %s to %s: src not found", oldName, newName));
-    }
-    if (fs.exists(newName)) {
-      if (overwrite) {
-        HadoopUtils.moveToTrash(fs, newName);
-      } else {
-        throw new FileAlreadyExistsException(
-            String.format("Failed to rename %s to %s: dst already exists", oldName, newName));
+    //In default implementation of rename with rewrite option in FileSystem, if the parent dir of dst does not exist, it will throw exception,
+    //Which will fail some of our job unintentionally. So we only call that method when fs is an instance of DistributedFileSystem to avoid inconsistency problem
+    if(fs instanceof DistributedFileSystem) {
+      Options.Rename renameOptions = (overwrite) ? Options.Rename.OVERWRITE : Options.Rename.NONE;
+      FileSystemAdapter.rename(fs, oldName, newName, renameOptions);

Review comment:
       Do we need the FileSystemAdapter class? Can't we directly call fs.rename(src,dest,options) here?

##########
File path: gobblin-core/src/main/java/org/apache/gobblin/writer/FsDataWriter.java
##########
@@ -262,7 +262,7 @@ public void commit()
     LOG.info(String.format("Moving data from %s to %s", this.stagingFile, this.outputFile));
     // For the same reason as deleting the staging file if it already exists, overwrite
     // the output file if it already exists to prevent task retry from being blocked.
-    HadoopUtils.renamePath(this.fileContext, this.stagingFile, this.outputFile, true);
+    HadoopUtils.renamePath(this.fs, this.stagingFile, this.outputFile, true);

Review comment:
       Did you forget to remove the fileContext member variable from this class?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org