(gobblin) branch master updated: [GOBBLIN-2043] Set execute bit only for new folders in manifest distcp (#3922)

[wl...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

wlo pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/gobblin.git


The following commit(s) were added to refs/heads/master by this push:
     new e79375a55 [GOBBLIN-2043] Set execute bit only for new folders in manifest distcp (#3922)
e79375a55 is described below

commit e79375a555e96b6df2a79b7bb1a5b7fa309d1538
Author: William Lo <lo...@gmail.com>
AuthorDate: Wed Apr 17 15:48:53 2024 -0400

    [GOBBLIN-2043] Set execute bit only for new folders in manifest distcp (#3922)
    
    Set execute bit only for new folders in manifest distcp
---
 .../data/management/copy/ManifestBasedDataset.java  | 21 +++++++++++++++++----
 .../util/commit/SetPermissionCommitStep.java        |  2 ++
 .../dataset/ManifestBasedDatasetFinderTest.java     |  3 +++
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/ManifestBasedDataset.java b/gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/ManifestBasedDataset.java
index b81d59663..c6823d39b 100644
--- a/gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/ManifestBasedDataset.java
+++ b/gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/ManifestBasedDataset.java
@@ -26,10 +26,12 @@ import com.google.gson.JsonIOException;
 import com.google.gson.JsonSyntaxException;
 import java.io.IOException;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Properties;
+import java.util.Set;
 import java.util.TreeMap;
 import java.util.concurrent.TimeUnit;
 import lombok.extern.slf4j.Slf4j;
@@ -37,6 +39,7 @@ import org.apache.gobblin.commit.CommitStep;
 import org.apache.gobblin.data.management.copy.entities.PostPublishStep;
 import org.apache.gobblin.data.management.copy.entities.PrePublishStep;
 import org.apache.gobblin.data.management.partition.FileSet;
+import org.apache.gobblin.util.PathUtils;
 import org.apache.gobblin.util.commit.DeleteFileCommitStep;
 import org.apache.gobblin.util.commit.SetPermissionCommitStep;
 
@@ -130,10 +133,12 @@ public class ManifestBasedDataset implements IterableCopyableDataset {
             copyEntities.add(copyableFile);
 
             Path fromPath = srcFs.getFileStatus(fileToCopy).isDirectory() ? fileToCopy : fileToCopy.getParent();
-
-            ancestorOwnerAndPermissions.putAll(
-                CopyableFile.resolveReplicatedAncestorOwnerAndPermissionsRecursively(srcFs, fromPath,
-                    new Path(commonFilesParent), configuration));
+            // Avoid duplicate calculation for the same ancestor
+            if (!ancestorOwnerAndPermissions.containsKey(PathUtils.getPathWithoutSchemeAndAuthority(fromPath).toString())) {
+              ancestorOwnerAndPermissions.putAll(
+                  CopyableFile.resolveReplicatedAncestorOwnerAndPermissionsRecursively(srcFs, fromPath,
+                      new Path(commonFilesParent), configuration));
+            }
 
             if (existOnTarget && srcFile.isFile()) {
               // this is to match the existing publishing behavior where we won't rewrite the target when it's already existed
@@ -146,6 +151,14 @@ public class ManifestBasedDataset implements IterableCopyableDataset {
         }
       }
 
+      // Only set permission for newly created folders on target
+      // To change permissions for existing dirs, expectation is to add the folder to the manifest
+      Set<String> parentFolders = new HashSet<>(ancestorOwnerAndPermissions.keySet());
+      for (String folder : parentFolders) {
+        if (targetFs.exists(new Path(folder))) {
+          ancestorOwnerAndPermissions.remove(folder);
+        }
+      }
       Properties props = new Properties();
       props.setProperty(SetPermissionCommitStep.STOP_ON_ERROR_KEY, "true");
       CommitStep setPermissionCommitStep = new SetPermissionCommitStep(targetFs, ancestorOwnerAndPermissions, props);
diff --git a/gobblin-data-management/src/main/java/org/apache/gobblin/util/commit/SetPermissionCommitStep.java b/gobblin-data-management/src/main/java/org/apache/gobblin/util/commit/SetPermissionCommitStep.java
index dd1a68bb4..0887c05bf 100644
--- a/gobblin-data-management/src/main/java/org/apache/gobblin/util/commit/SetPermissionCommitStep.java
+++ b/gobblin-data-management/src/main/java/org/apache/gobblin/util/commit/SetPermissionCommitStep.java
@@ -27,6 +27,7 @@ import org.apache.hadoop.fs.FileSystem;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.security.AccessControlException;
 
+import lombok.Getter;
 import lombok.extern.slf4j.Slf4j;
 
 import org.apache.gobblin.commit.CommitStep;
@@ -38,6 +39,7 @@ import org.apache.gobblin.data.management.copy.OwnerAndPermission;
  */
 @Slf4j
 public class SetPermissionCommitStep implements CommitStep {
+  @Getter
   Map<String, OwnerAndPermission> pathAndPermissions;
   private final URI fsUri;
   public final boolean stopOnError;
diff --git a/gobblin-data-management/src/test/java/org/apache/gobblin/data/management/dataset/ManifestBasedDatasetFinderTest.java b/gobblin-data-management/src/test/java/org/apache/gobblin/data/management/dataset/ManifestBasedDatasetFinderTest.java
index a0dbcacad..1adbe9ea3 100644
--- a/gobblin-data-management/src/test/java/org/apache/gobblin/data/management/dataset/ManifestBasedDatasetFinderTest.java
+++ b/gobblin-data-management/src/test/java/org/apache/gobblin/data/management/dataset/ManifestBasedDatasetFinderTest.java
@@ -93,6 +93,8 @@ public class ManifestBasedDatasetFinderTest {
       FileSet<CopyEntity> fileSet = fileSets.next();
       Assert.assertEquals(fileSet.getFiles().size(), 3);  // 2 files to copy + 1 post publish step
       Assert.assertTrue(((PostPublishStep) fileSet.getFiles().get(2)).getStep() instanceof SetPermissionCommitStep);
+      SetPermissionCommitStep step = (SetPermissionCommitStep) ((PostPublishStep) fileSet.getFiles().get(2)).getStep();
+      Assert.assertEquals(step.getPathAndPermissions().keySet().size(), 2);
       Mockito.verify(manifestReadFs, Mockito.times(1)).exists(manifestPath);
       Mockito.verify(manifestReadFs, Mockito.times(1)).getFileStatus(manifestPath);
       Mockito.verify(manifestReadFs, Mockito.times(1)).open(manifestPath);
@@ -178,6 +180,7 @@ public class ManifestBasedDatasetFinderTest {
     Mockito.when(sourceFs.getUri()).thenReturn(SRC_FS_URI);
     Mockito.when(manifestReadFs.getUri()).thenReturn(MANIFEST_READ_FS_URI);
     Mockito.when(destFs.getUri()).thenReturn(DEST_FS_URI);
+    Mockito.when(destFs.exists(new Path("/tmp"))).thenReturn(true);
     Mockito.when(sourceFs.getFileStatus(any(Path.class))).thenReturn(localFs.getFileStatus(new Path(tmpDir.toString())));
     Mockito.when(sourceFs.exists(any(Path.class))).thenReturn(true);
     Mockito.when(manifestReadFs.exists(any(Path.class))).thenReturn(true);