[GitHub] [gobblin] phet commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp

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

phet commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1067725962


##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/PreserveAttributes.java:
##########
@@ -45,6 +45,8 @@ public static enum Option {
     GROUP('g'),
     PERMISSION('p'),
     VERSION('v'),
+    ACL('a'),
+    STICKY_BIT('s'),

Review Comment:
   it was a couple of weeks back, but didn't we discuss the sticky bit and conclude that it's not necessary to track specifically, as it is already encompassed by the `PERMISSION`s?
   
   is there an specific use case where we'd wish to control them independently?



##########
gobblin-data-management/src/main/java/gobblin/data/management/copy/OwnerAndPermission.java:
##########
@@ -18,12 +18,13 @@
 
 import org.apache.hadoop.fs.permission.FsPermission;
 
+import com.google.api.client.util.Lists;
 
 /***
  * Shim layer for org.apache.gobblin.data.management.copy.OwnerAndPermission
  */
 public class OwnerAndPermission extends org.apache.gobblin.data.management.copy.OwnerAndPermission {
   public OwnerAndPermission(String owner, String group, FsPermission fsPermission) {
-    super(owner, group, fsPermission);
+    super(owner, group, fsPermission, null, Lists.newArrayList());

Review Comment:
   didn't you retain a three-arg version of the superclass's constructor?  why do you need this change here?



##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -481,4 +497,14 @@ public void cleanup() {
     }
   }
 
+  protected static class TestFileAwareInputStreamDataWriter extends FileAwareInputStreamDataWriter {
+
+    public TestFileAwareInputStreamDataWriter(State state, int numBranches, int branchId) throws IOException {
+      super(state, numBranches, branchId, null);
+    }
+    @Override
+    protected void setAclOnPath(FileSystem theFs, Path path, List<AclEntry> aclEntries) {

Review Comment:
   aah... I see you do plan to override.
   
   to be very clear: the original method absolutely, 100% deserves javadoc justifying what otherwise appears a superfluous abstraction.



##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriter.java:
##########
@@ -495,20 +512,32 @@ private void ensureDirectoryExists(FileSystem fs, Path path, Iterator<OwnerAndPe
 
       if (ownerAndPermission.getFsPermission() != null) {
         log.debug("Applying permissions {} to path {}.", ownerAndPermission.getFsPermission(), path);
+        if (ownerAndPermission.getStickyBit() != null) {
+          FsPermission fsPermissionWithStickyBit = getFsPermissionWithStickyBit(ownerAndPermission);
+          ownerAndPermission.setFsPermission(fsPermissionWithStickyBit);
+        }
         fs.setPermission(path, addExecutePermissionToOwner(ownerAndPermission.getFsPermission()));
       }
 
       String group = ownerAndPermission.getGroup();
       String owner = ownerAndPermission.getOwner();
+      List<AclEntry> aclEntries = ownerAndPermission.getAclEntries();
       if (group != null || owner != null) {
         log.debug("Applying owner {} and group {} to path {}.", owner, group, path);
         fs.setOwner(path, owner, group);
       }
+      if (!aclEntries.isEmpty()) {
+        setAclOnPath(fs, path, aclEntries);

Review Comment:
   usually I am one for abstraction, but here, I wonder what does it accomplish beyond:
   ```
   fs.setAcl(path, aclEntries)
   ```
   ?
   e.g. did you leave `setAclOnPath` non-`final` for a reason, anticipating a valid reason to override in a derived class?



##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/TestCopyableDataset.java:
##########
@@ -43,7 +43,7 @@ public class TestCopyableDataset implements CopyableDataset, FileSystemDataset {
   public static final String DESTINATION_PREFIX = "/destination";
   public static final String RELATIVE_PREFIX = "/relative";
   public static final OwnerAndPermission OWNER_AND_PERMISSION = new OwnerAndPermission("owner", "group",
-      FsPermission.getDefault());
+      FsPermission.getDefault(), null, Lists.newArrayList());

Review Comment:
   again... didn't you retain a three-arg version?



##########
gobblin-data-management/src/test/java/org/apache/gobblin/util/commit/SetPermissionCommitStepTest.java:
##########
@@ -55,7 +57,8 @@ public void setUp() throws IOException {
     dir1 = new Path(ROOT_DIR, "dir1");
     this.fs.mkdirs(dir1);
 
-    OwnerAndPermission ownerAndPermission = new OwnerAndPermission("owner", "group", permission);
+    OwnerAndPermission ownerAndPermission = new OwnerAndPermission("owner", "group", permission, null,
+        Lists.newArrayList());

Review Comment:
   once more: on more than a few of these... what about the three-arg version you actually retained?



##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -481,4 +497,14 @@ public void cleanup() {
     }
   }
 
+  protected static class TestFileAwareInputStreamDataWriter extends FileAwareInputStreamDataWriter {
+
+    public TestFileAwareInputStreamDataWriter(State state, int numBranches, int branchId) throws IOException {
+      super(state, numBranches, branchId, null);
+    }
+    @Override
+    protected void setAclOnPath(FileSystem theFs, Path path, List<AclEntry> aclEntries) {

Review Comment:
   also, with an empty/no-op impl, there doesn't appear to be any way to validate test correctness.  do you also perceive the gap?



##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriter.java:
##########
@@ -391,6 +398,14 @@ private void setRecursivePermission(Path path, OwnerAndPermission ownerAndPermis
     }
   }
 
+  private FsPermission getFsPermissionWithStickyBit(OwnerAndPermission ownerAndPermission) {
+    FsPermission fsPermission = ownerAndPermission.getFsPermission();
+    FsPermission fsPermissionWithStickyBit = new FsPermission(fsPermission.getUserAction(), fsPermission.getGroupAction(), fsPermission.getOtherAction(),
+        ownerAndPermission.getStickyBit());
+    ownerAndPermission.setFsPermission(fsPermissionWithStickyBit);

Review Comment:
   (I continue to have the same questions I asked previously)



##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -339,7 +345,17 @@ public void testCommit() throws IOException {
     FileStatus status = this.fs.getFileStatus(originFile);
     FsPermission readWrite = new FsPermission(FsAction.READ_WRITE, FsAction.READ_WRITE, FsAction.READ_WRITE);
     FsPermission dirReadWrite = new FsPermission(FsAction.ALL, FsAction.READ_WRITE, FsAction.READ_WRITE);
-    OwnerAndPermission ownerAndPermission = new OwnerAndPermission(status.getOwner(), status.getGroup(), readWrite);
+    AclEntry aclEntry = new AclEntry.Builder()
+        .setPermission(FsAction.READ_WRITE)
+        .setName("test-acl")
+        .setScope(AclEntryScope.DEFAULT)
+        .setType(AclEntryType.GROUP)
+        .build();
+
+    List<AclEntry> aclEntryList = Lists.newArrayList();
+    aclEntryList.add(aclEntry);

Review Comment:
   while it is passed as a param to `OwnerAndPermission`'s ctor... to actually test something, shouldn't it participate in some validation?



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

To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org

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