phet commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1050273518
##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriter.java:
##########
@@ -357,8 +358,15 @@ private void safeSetPathPermission(Path path,
OwnerAndPermission ownerAndPermiss
try {
if (ownerAndPermission.getFsPermission() != null) {
+ if (ownerAndPermission.getStickyBit() != null) {
+ FsPermission fsPermissionWithStickyBit =
getFsPermissionWithStickyBit(ownerAndPermission);
+ ownerAndPermission.setFsPermission(fsPermissionWithStickyBit);
+ }
this.fs.setPermission(path, ownerAndPermission.getFsPermission());
}
+ if (!ownerAndPermission.getAclEntries().isEmpty()) {
Review Comment:
hmmm.. looking above that it's possible to construct `OwnerAndPermission`w/o
supplying an `aclEntries` list, wouldn't that leave it `null`, such that this
would be an NPE?
##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/OwnerAndPermission.java:
##########
@@ -42,6 +44,14 @@ public class OwnerAndPermission implements Writable {
private String owner;
private String group;
private FsPermission fsPermission;
+ private Boolean stickyBit;
+ private List<AclEntry> aclEntries;
+
+ public OwnerAndPermission (String owner, String group, FsPermission
fsPermission) {
+ this.owner = owner;
+ this.group = group;
+ this.fsPermission = fsPermission;
+ }
Review Comment:
curious: why retain this? why not update the callers using it to use the now
five-arg ctor?
##########
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:
it's typically an anti-pattern to update params within some operation (other
than a method on the instance being modified). consider: how or why would the
caller ever suspect this?
even so, I'm unclear what that accomplishes, since what you set it to
appears to have been initialized by what it would replace... (very confused)
please describe what you're doing here.
##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriter.java:
##########
@@ -495,15 +510,23 @@ 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()) {
Review Comment:
again, missing `null` check
##########
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:
is `aclEntryList` even used? ...or is the diff not flagging another change
elsewhere?!?!?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
