You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by "ASF GitHub Bot (Jira)" <ji...@apache.org> on 2023/01/12 06:12:00 UTC
[jira] [Work logged] (GOBBLIN-1755) Support extended ACLs and sticky bit for file based distcp
[ https://issues.apache.org/jira/browse/GOBBLIN-1755?focusedWorklogId=838768&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-838768 ]
ASF GitHub Bot logged work on GOBBLIN-1755:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 12/Jan/23 06:11
Start Date: 12/Jan/23 06:11
Worklog Time Spent: 10m
Work Description: 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?
Issue Time Tracking
-------------------
Worklog Id: (was: 838768)
Time Spent: 40m (was: 0.5h)
> Support extended ACLs and sticky bit for file based distcp
> ----------------------------------------------------------
>
> Key: GOBBLIN-1755
> URL: https://issues.apache.org/jira/browse/GOBBLIN-1755
> Project: Apache Gobblin
> Issue Type: Task
> Reporter: Meeth Gala
> Priority: Major
> Time Spent: 40m
> Remaining Estimate: 0h
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)