You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by GitBox <gi...@apache.org> on 2022/12/14 06:14:00 UTC
[GitHub] [gobblin] meethngala opened a new pull request, #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
meethngala opened a new pull request, #3616:
URL: https://github.com/apache/gobblin/pull/3616
Dear Gobblin maintainers,
Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
### JIRA
- [ ] My PR addresses the following [Gobblin JIRA](https://issues.apache.org/jira/browse/GOBBLIN/) issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
- https://issues.apache.org/jira/browse/GOBBLIN-1755
### Description
- [ ] Here are some details about my PR, including screenshots (if applicable):
- [ ] Added new preserve attributes for extended ACLs and sticky bit support
- [ ] Updated FileAwareInputStreamDataWriter to set ACLs and sticky bit
### Tests
- [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
- [ ] build successfully
### Commits
- [ ] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"
--
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
[GitHub] [gobblin] meethngala commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by "meethngala (via GitHub)" <gi...@apache.org>.
meethngala commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1095293097
##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriter.java:
##########
@@ -500,15 +504,26 @@ private void ensureDirectoryExists(FileSystem fs, Path path, Iterator<OwnerAndPe
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);
+ }
} else {
fs.mkdirs(path);
}
}
+ /*
+ * Creating an abstraction layer to support unit testing on raw local file system for validating if ACLs are set on a file path
Review Comment:
I no longer use this stubbing and have extended the LocalFileSystem to support the setAcl method.
--
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
[GitHub] [gobblin] phet commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1097909721
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -523,11 +509,14 @@ public void cleanup() {
}
}
- protected static class TestLocalFileSystem extends LocalFileSystem {
-
+ protected class TestLocalFileSystem extends LocalFileSystem {
+ ConcurrentHashMap<Path, List<AclEntry>> pathToAclEntries = new ConcurrentHashMap<>();
@Override
public void setAcl(Path path, List<AclEntry> aclEntries) {
pathToAclEntries.put(path, aclEntries);
}
+ public ConcurrentHashMap<Path, List<AclEntry>> getPathToAclEntries() {
+ return pathToAclEntries;
Review Comment:
first off, I recognize this is just a unit test... but still I'd look for it to reach at least somewhere on this spectrum:
1. the return type shouldn't be `ConcurrentHashMap`--because the caller's not supposed to write to it!
2. return `ImmutableMap` so the caller breaks on any misguided attempt to write to it (even though not isolated from concurrent puts)
3. best practice of a defensive copy by returning a new map (to freeze it at a point in time). here, should be `synchronized` to avoid concurrent modification exception during iteration
--
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
[GitHub] [gobblin] Will-Lo merged pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by "Will-Lo (via GitHub)" <gi...@apache.org>.
Will-Lo merged PR #3616:
URL: https://github.com/apache/gobblin/pull/3616
--
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
[GitHub] [gobblin] meethngala commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by "meethngala (via GitHub)" <gi...@apache.org>.
meethngala commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1097760162
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -402,8 +423,29 @@ public void testCommit() throws IOException {
// previously existing paths should not have permissions changed
fileStatus = this.fs.getFileStatus(existingOutputPath);
Assert.assertEquals(fileStatus.getPermission(), existingPathPermission);
-
+ verifyAclEntries(writer, pathToAclEntries, expectedOutputPath);
Assert.assertFalse(this.fs.exists(writer.stagingDir));
+
+ }
+
+ private void verifyAclEntries(FileAwareInputStreamDataWriter writer, ConcurrentHashMap pathToAclEntries, Path expectedOutputPath) {
+ // fetching and preparing file paths from FileAwareInputStreamDataWriter object
+ Path stagingDir = writer.stagingDir;
+ Path outputDir = writer.outputDir;
+ String[] splitExpectedOutputPath = expectedOutputPath.toString().split("output");
+ Path dstOutputPath = new Path(outputDir.toString().concat(splitExpectedOutputPath[1])).getParent();
+ Path stgFilePath = new Path("file:".concat(stagingDir.toString().concat("/file")));
Review Comment:
`String.concat` handles NPEs if any compared to `+` ... I guess that's the only difference.
--
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
[GitHub] [gobblin] meethngala commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by "meethngala (via GitHub)" <gi...@apache.org>.
meethngala commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1099619791
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -523,11 +509,14 @@ public void cleanup() {
}
}
- protected static class TestLocalFileSystem extends LocalFileSystem {
-
+ protected class TestLocalFileSystem extends LocalFileSystem {
+ ConcurrentHashMap<Path, List<AclEntry>> pathToAclEntries = new ConcurrentHashMap<>();
Review Comment:
yeah... my bad! Updated the PR with the changes
--
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
[GitHub] [gobblin] meethngala commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by "meethngala (via GitHub)" <gi...@apache.org>.
meethngala commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1099620238
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -423,29 +413,25 @@ public void testCommit() throws IOException {
// previously existing paths should not have permissions changed
fileStatus = this.fs.getFileStatus(existingOutputPath);
Assert.assertEquals(fileStatus.getPermission(), existingPathPermission);
- verifyAclEntries(writer, pathToAclEntries, expectedOutputPath);
+ verifyAclEntries(writer, this.fs.getPathToAclEntries(), expectedOutputPath);
Assert.assertFalse(this.fs.exists(writer.stagingDir));
}
private void verifyAclEntries(FileAwareInputStreamDataWriter writer, ConcurrentHashMap pathToAclEntries, Path expectedOutputPath) {
// fetching and preparing file paths from FileAwareInputStreamDataWriter object
- Path stagingDir = writer.stagingDir;
Path outputDir = writer.outputDir;
String[] splitExpectedOutputPath = expectedOutputPath.toString().split("output");
Path dstOutputPath = new Path(outputDir.toString().concat(splitExpectedOutputPath[1])).getParent();
- Path stgFilePath = new Path("file:".concat(stagingDir.toString().concat("/file")));
-
Assert.assertTrue(pathToAclEntries.containsKey(dstOutputPath));
- Assert.assertTrue(pathToAclEntries.containsKey(stgFilePath));
OwnerAndPermission destinationOwnerAndPermission = writer.actualProcessedCopyableFile.get().getDestinationOwnerAndPermission();
List<AclEntry> actual = destinationOwnerAndPermission.getAclEntries();
- List<AclEntry> expectedStgAclEntries = (List<AclEntry>) pathToAclEntries.get(stgFilePath);
- List<AclEntry> expectedDstAclEntries = (List<AclEntry>) pathToAclEntries.get(dstOutputPath);
-
- Assert.assertEquals(actual, expectedStgAclEntries);
- Assert.assertEquals(actual, expectedDstAclEntries);
+ do {
+ List<AclEntry> expected = (List<AclEntry>) pathToAclEntries.get(dstOutputPath);
+ Assert.assertEquals(actual, expected);
+ dstOutputPath = dstOutputPath.getParent();
+ } while (pathToAclEntries.containsKey(dstOutputPath));
Review Comment:
makes sense! my latest commit handles this issue... thanks for the feedback!
--
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
[GitHub] [gobblin] meethngala commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by "meethngala (via GitHub)" <gi...@apache.org>.
meethngala commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1099626433
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -402,10 +409,169 @@ public void testCommit() throws IOException {
// previously existing paths should not have permissions changed
fileStatus = this.fs.getFileStatus(existingOutputPath);
Assert.assertEquals(fileStatus.getPermission(), existingPathPermission);
-
Assert.assertFalse(this.fs.exists(writer.stagingDir));
}
+ @Test
+ public void testCommitWithAclPreservationWhenAncestorPathsAbsent() throws IOException {
+ String fileName = "file";
+ // Asemble destination paths
+ String destinationExistingToken = "destination";
+ String destinationAdditionalTokens = "path";
+ Path destination = new Path(new Path(new Path("/", destinationExistingToken), destinationAdditionalTokens), fileName);
+ Path destinationWithoutLeadingSeparator = new Path(new Path(destinationExistingToken, destinationAdditionalTokens), fileName);
Review Comment:
ahh... that's a good catch. I essentially borrowed these paths from old test cases... I have made the changes and pushed a latest commit for the same!
--
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
[GitHub] [gobblin] meethngala commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by "meethngala (via GitHub)" <gi...@apache.org>.
meethngala commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1095290139
##########
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:
latest commit takes care of this
--
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
[GitHub] [gobblin] meethngala commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by "meethngala (via GitHub)" <gi...@apache.org>.
meethngala commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1097760828
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -402,8 +423,29 @@ public void testCommit() throws IOException {
// previously existing paths should not have permissions changed
fileStatus = this.fs.getFileStatus(existingOutputPath);
Assert.assertEquals(fileStatus.getPermission(), existingPathPermission);
-
+ verifyAclEntries(writer, pathToAclEntries, expectedOutputPath);
Assert.assertFalse(this.fs.exists(writer.stagingDir));
+
+ }
+
+ private void verifyAclEntries(FileAwareInputStreamDataWriter writer, ConcurrentHashMap pathToAclEntries, Path expectedOutputPath) {
+ // fetching and preparing file paths from FileAwareInputStreamDataWriter object
+ Path stagingDir = writer.stagingDir;
Review Comment:
yes, we don't have to. I have updated my PR
--
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
[GitHub] [gobblin] meethngala commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by "meethngala (via GitHub)" <gi...@apache.org>.
meethngala commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1097761781
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -481,4 +523,11 @@ public void cleanup() {
}
}
-}
+ protected static class TestLocalFileSystem extends LocalFileSystem {
Review Comment:
made the changes requested above in my latest commit
--
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
[GitHub] [gobblin] meethngala commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by "meethngala (via GitHub)" <gi...@apache.org>.
meethngala commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1095293284
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -481,4 +499,15 @@ public void cleanup() {
}
}
+ protected static class TestFileAwareInputStreamDataWriter extends FileAwareInputStreamDataWriter {
+
+ static boolean isAclSet = false;
Review Comment:
I have added the verification logic based on your advice in my latest commit!
--
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
[GitHub] [gobblin] meethngala commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by "meethngala (via GitHub)" <gi...@apache.org>.
meethngala commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1097761046
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -380,12 +401,12 @@ public void testCommit() throws IOException {
FileStatus fileStatus = this.fs.getFileStatus(existingOutputPath);
FsPermission existingPathPermission = fileStatus.getPermission();
+
Review Comment:
yeah... seems like it!
--
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
[GitHub] [gobblin] phet commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by 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
[GitHub] [gobblin] Will-Lo commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by GitBox <gi...@apache.org>.
Will-Lo commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1069681531
##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriter.java:
##########
@@ -500,15 +504,26 @@ private void ensureDirectoryExists(FileSystem fs, Path path, Iterator<OwnerAndPe
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);
+ }
} else {
fs.mkdirs(path);
}
}
+ /*
+ * Creating an abstraction layer to support unit testing on raw local file system for validating if ACLs are set on a file path
+ */
+ protected void setAclOnPath(FileSystem theFs, Path path, List<AclEntry> aclEntries) throws IOException {
+ theFs.setAcl(path, aclEntries);
+ }
Review Comment:
Question, does LocalFileSystem in Hadoop support setAcl and a reader on acls? It could be more straightforward to test directly using set and get acls on a random file you write locally.
If not this is fine
--
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
[GitHub] [gobblin] phet commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by GitBox <gi...@apache.org>.
phet commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1069857107
##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/PreserveAttributes.java:
##########
@@ -45,6 +45,7 @@ public static enum Option {
GROUP('g'),
PERMISSION('p'),
VERSION('v'),
+ ACL('a'),
Review Comment:
nit: shouldn't this be plural, `ACLS`, since it preserves a list of them?
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -481,4 +499,15 @@ public void cleanup() {
}
}
+ protected static class TestFileAwareInputStreamDataWriter extends FileAwareInputStreamDataWriter {
+
+ static boolean isAclSet = false;
Review Comment:
this is very weak verification... also the class-level `static` is an anti-pattern in tests (as most framework runners can work concurrently).
better would be an in-memory map from filepath to a set of acls. the verification code would ask this mock object to provide the acls it recorded for each path, and then verification would ensure those meet the expectations
##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriter.java:
##########
@@ -500,15 +504,26 @@ private void ensureDirectoryExists(FileSystem fs, Path path, Iterator<OwnerAndPe
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);
+ }
} else {
fs.mkdirs(path);
}
}
+ /*
+ * Creating an abstraction layer to support unit testing on raw local file system for validating if ACLs are set on a file path
Review Comment:
@Will-Lo asked below, which probably indicates this is not stated clearly enough. the motivation arises because the `LocalFileSystem` lacks support--do be sure to say so
also, this should be javadoc comment to show up there.
--
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
[GitHub] [gobblin] phet commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1095350586
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -402,8 +423,29 @@ public void testCommit() throws IOException {
// previously existing paths should not have permissions changed
fileStatus = this.fs.getFileStatus(existingOutputPath);
Assert.assertEquals(fileStatus.getPermission(), existingPathPermission);
-
+ verifyAclEntries(writer, pathToAclEntries, expectedOutputPath);
Assert.assertFalse(this.fs.exists(writer.stagingDir));
+
+ }
+
+ private void verifyAclEntries(FileAwareInputStreamDataWriter writer, ConcurrentHashMap pathToAclEntries, Path expectedOutputPath) {
+ // fetching and preparing file paths from FileAwareInputStreamDataWriter object
+ Path stagingDir = writer.stagingDir;
+ Path outputDir = writer.outputDir;
+ String[] splitExpectedOutputPath = expectedOutputPath.toString().split("output");
+ Path dstOutputPath = new Path(outputDir.toString().concat(splitExpectedOutputPath[1])).getParent();
+ Path stgFilePath = new Path("file:".concat(stagingDir.toString().concat("/file")));
Review Comment:
don't think I've ever actually used the `String.concat` method before... what about the canonical `+`? is there some advantage, since:
```
new Path("file:" + stagingDir + "/file");
```
seems easier to read?
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -402,8 +423,29 @@ public void testCommit() throws IOException {
// previously existing paths should not have permissions changed
fileStatus = this.fs.getFileStatus(existingOutputPath);
Assert.assertEquals(fileStatus.getPermission(), existingPathPermission);
-
+ verifyAclEntries(writer, pathToAclEntries, expectedOutputPath);
Assert.assertFalse(this.fs.exists(writer.stagingDir));
+
+ }
+
+ private void verifyAclEntries(FileAwareInputStreamDataWriter writer, ConcurrentHashMap pathToAclEntries, Path expectedOutputPath) {
+ // fetching and preparing file paths from FileAwareInputStreamDataWriter object
+ Path stagingDir = writer.stagingDir;
Review Comment:
not immediately seeing... why do we need to verify staging? isn't dest the only one that matters?
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -65,17 +70,22 @@
import org.apache.gobblin.util.WriterUtils;
import org.apache.gobblin.util.io.StreamUtils;
-import static org.mockito.Mockito.*;
+import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.doCallRealMethod;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.spy;
public class FileAwareInputStreamDataWriterTest {
FileSystem fs;
Path testTempPath;
+ public static ConcurrentHashMap<Path, List<AclEntry>> pathToAclEntries;
Review Comment:
I mentioned before and still advise not to use `static` / class state in unit tests, since parallel test runners may give odd results.
also, consider: if `fs` is instance-level and `pathToAclEntries` is here to augment that, why wouldn't we want that instance-level too?
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -481,4 +523,11 @@ public void cleanup() {
}
}
-}
+ protected static class TestLocalFileSystem extends LocalFileSystem {
Review Comment:
why not encapsulate the `ConcurrentHashMap` here? just add a method to access it for verification (copying-on-write would be most typical).
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -402,8 +423,29 @@ public void testCommit() throws IOException {
// previously existing paths should not have permissions changed
fileStatus = this.fs.getFileStatus(existingOutputPath);
Assert.assertEquals(fileStatus.getPermission(), existingPathPermission);
-
+ verifyAclEntries(writer, pathToAclEntries, expectedOutputPath);
Review Comment:
reqs-wise, how does this feature treat ACLs on parent dirs that it's creating--will they be preserved or not? a unit test should codify "the spec". hence, either way, shouldn't we exercise that scenario to assert it's handled in the proper way?
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -380,12 +401,12 @@ public void testCommit() throws IOException {
FileStatus fileStatus = this.fs.getFileStatus(existingOutputPath);
FsPermission existingPathPermission = fileStatus.getPermission();
+
Review Comment:
snuck in?
--
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
[GitHub] [gobblin] meethngala commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by "meethngala (via GitHub)" <gi...@apache.org>.
meethngala commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1095289381
##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/PreserveAttributes.java:
##########
@@ -45,6 +45,7 @@ public static enum Option {
GROUP('g'),
PERMISSION('p'),
VERSION('v'),
+ ACL('a'),
Review Comment:
I have updated the file with the above change in my latest commit
--
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
[GitHub] [gobblin] meethngala commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by "meethngala (via GitHub)" <gi...@apache.org>.
meethngala commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1095295658
##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriter.java:
##########
@@ -500,15 +504,26 @@ private void ensureDirectoryExists(FileSystem fs, Path path, Iterator<OwnerAndPe
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);
+ }
} else {
fs.mkdirs(path);
}
}
+ /*
+ * Creating an abstraction layer to support unit testing on raw local file system for validating if ACLs are set on a file path
+ */
+ protected void setAclOnPath(FileSystem theFs, Path path, List<AclEntry> aclEntries) throws IOException {
+ theFs.setAcl(path, aclEntries);
+ }
Review Comment:
Yes, I agree. I have extended LocalFileSystem to support setAcl and handle it accordingly for the purpose of unit testing in my latest commit. I no longer create this additional layer of abstraction.
--
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
[GitHub] [gobblin] meethngala commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by "meethngala (via GitHub)" <gi...@apache.org>.
meethngala commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1099620017
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -523,11 +509,14 @@ public void cleanup() {
}
}
- protected static class TestLocalFileSystem extends LocalFileSystem {
-
+ protected class TestLocalFileSystem extends LocalFileSystem {
+ ConcurrentHashMap<Path, List<AclEntry>> pathToAclEntries = new ConcurrentHashMap<>();
@Override
public void setAcl(Path path, List<AclEntry> aclEntries) {
pathToAclEntries.put(path, aclEntries);
}
+ public ConcurrentHashMap<Path, List<AclEntry>> getPathToAclEntries() {
+ return pathToAclEntries;
Review Comment:
gotcha... thanks for the detailed explanation! I have updated my PR based on your feedback!
--
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
[GitHub] [gobblin] phet commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by GitBox <gi...@apache.org>.
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] meethngala commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by "meethngala (via GitHub)" <gi...@apache.org>.
meethngala commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1097760489
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -65,17 +70,22 @@
import org.apache.gobblin.util.WriterUtils;
import org.apache.gobblin.util.io.StreamUtils;
-import static org.mockito.Mockito.*;
+import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.doCallRealMethod;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.spy;
public class FileAwareInputStreamDataWriterTest {
FileSystem fs;
Path testTempPath;
+ public static ConcurrentHashMap<Path, List<AclEntry>> pathToAclEntries;
Review Comment:
I have made the above changes in my latest PR
--
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
[GitHub] [gobblin] meethngala commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by "meethngala (via GitHub)" <gi...@apache.org>.
meethngala commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1095291874
##########
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:
latest commit doesn't require this abstraction... since the only reason we had this stubbing was to make unit testing easier. But now, after the latest changes I directly call setAcl and override the localFs in the unit test
--
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
[GitHub] [gobblin] meethngala commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by "meethngala (via GitHub)" <gi...@apache.org>.
meethngala commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1095292630
##########
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:
latest commit takes care of this!
--
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
[GitHub] [gobblin] meethngala commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by "meethngala (via GitHub)" <gi...@apache.org>.
meethngala commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1095289972
##########
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:
I got rid of the stubbing altogether in my latest commit since I had to accommodate for changes related to manifest dataset and have updated the unit tests accordingly
--
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
[GitHub] [gobblin] phet commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1097906043
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -423,29 +413,25 @@ public void testCommit() throws IOException {
// previously existing paths should not have permissions changed
fileStatus = this.fs.getFileStatus(existingOutputPath);
Assert.assertEquals(fileStatus.getPermission(), existingPathPermission);
- verifyAclEntries(writer, pathToAclEntries, expectedOutputPath);
+ verifyAclEntries(writer, this.fs.getPathToAclEntries(), expectedOutputPath);
Assert.assertFalse(this.fs.exists(writer.stagingDir));
}
private void verifyAclEntries(FileAwareInputStreamDataWriter writer, ConcurrentHashMap pathToAclEntries, Path expectedOutputPath) {
// fetching and preparing file paths from FileAwareInputStreamDataWriter object
- Path stagingDir = writer.stagingDir;
Path outputDir = writer.outputDir;
String[] splitExpectedOutputPath = expectedOutputPath.toString().split("output");
Path dstOutputPath = new Path(outputDir.toString().concat(splitExpectedOutputPath[1])).getParent();
- Path stgFilePath = new Path("file:".concat(stagingDir.toString().concat("/file")));
-
Assert.assertTrue(pathToAclEntries.containsKey(dstOutputPath));
- Assert.assertTrue(pathToAclEntries.containsKey(stgFilePath));
OwnerAndPermission destinationOwnerAndPermission = writer.actualProcessedCopyableFile.get().getDestinationOwnerAndPermission();
List<AclEntry> actual = destinationOwnerAndPermission.getAclEntries();
- List<AclEntry> expectedStgAclEntries = (List<AclEntry>) pathToAclEntries.get(stgFilePath);
- List<AclEntry> expectedDstAclEntries = (List<AclEntry>) pathToAclEntries.get(dstOutputPath);
-
- Assert.assertEquals(actual, expectedStgAclEntries);
- Assert.assertEquals(actual, expectedDstAclEntries);
+ do {
+ List<AclEntry> expected = (List<AclEntry>) pathToAclEntries.get(dstOutputPath);
+ Assert.assertEquals(actual, expected);
+ dstOutputPath = dstOutputPath.getParent();
+ } while (pathToAclEntries.containsKey(dstOutputPath));
Review Comment:
seems like the input to be verified is put in control of the verification itself. to avoid "cheating" the expected values should drive verification. e.g. if "actual" were the empty map (which would mean no parent dirs were handled properly in some cases) wouldn't this test still pass?
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -523,11 +509,14 @@ public void cleanup() {
}
}
- protected static class TestLocalFileSystem extends LocalFileSystem {
-
+ protected class TestLocalFileSystem extends LocalFileSystem {
+ ConcurrentHashMap<Path, List<AclEntry>> pathToAclEntries = new ConcurrentHashMap<>();
@Override
public void setAcl(Path path, List<AclEntry> aclEntries) {
pathToAclEntries.put(path, aclEntries);
}
+ public ConcurrentHashMap<Path, List<AclEntry>> getPathToAclEntries() {
+ return pathToAclEntries;
Review Comment:
first off, I recognize this is just a unit test... but still I'd look for it to reach at least somewhere on this spectrum:
1. the return type shouldn't be `ConcurrentHashMap`--because the caller's not supposed to write to it!
2. return `ImmutableMap` so the caller breaks on any misguided attempt to write to it (even though not isolated from concurrent puts)
3. best practice of copy-on-write by returning a new map.
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -523,11 +509,14 @@ public void cleanup() {
}
}
- protected static class TestLocalFileSystem extends LocalFileSystem {
-
+ protected class TestLocalFileSystem extends LocalFileSystem {
+ ConcurrentHashMap<Path, List<AclEntry>> pathToAclEntries = new ConcurrentHashMap<>();
Review Comment:
did you forget access modifier... package-protected isn't intended, is it?
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -523,11 +509,14 @@ public void cleanup() {
}
}
- protected static class TestLocalFileSystem extends LocalFileSystem {
-
+ protected class TestLocalFileSystem extends LocalFileSystem {
Review Comment:
it need not be long, but every class deserves its reason to exist documented with at least brief javadoc.
here, from my recollection, you met a problem because the `LocalFileSystem` doesn't pass through the ACL... hence this shim.
good work! ... but do please document that so maintainers need not puzzle over this and/or independently discover justification for the code you've added.
--
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
[GitHub] [gobblin] meethngala commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by "meethngala (via GitHub)" <gi...@apache.org>.
meethngala commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1099621031
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -402,10 +409,169 @@ public void testCommit() throws IOException {
// previously existing paths should not have permissions changed
fileStatus = this.fs.getFileStatus(existingOutputPath);
Assert.assertEquals(fileStatus.getPermission(), existingPathPermission);
-
Assert.assertFalse(this.fs.exists(writer.stagingDir));
}
+ @Test
+ public void testCommitWithAclPreservationWhenAncestorPathsAbsent() throws IOException {
+ String fileName = "file";
+ // Asemble destination paths
+ String destinationExistingToken = "destination";
+ String destinationAdditionalTokens = "path";
+ Path destination = new Path(new Path(new Path("/", destinationExistingToken), destinationAdditionalTokens), fileName);
+ Path destinationWithoutLeadingSeparator = new Path(new Path(destinationExistingToken, destinationAdditionalTokens), fileName);
+
+ // Create temp directory
+ File tmpFile = Files.createTempDir();
+ tmpFile.deleteOnExit();
+ Path tmpPath = new Path(tmpFile.getAbsolutePath());
+
+ // create origin file
+ Path originFile = new Path(tmpPath, fileName);
+ this.fs.createNewFile(originFile);
+
+ // create stating dir
+ Path stagingDir = new Path(tmpPath, "staging");
+ this.fs.mkdirs(stagingDir);
+
+ // create output dir
+ Path outputDir = new Path(tmpPath, "output");
+ this.fs.mkdirs(outputDir);
+
+ // create copyable file
+ CopyableFile cf = createCopyableFile(originFile, destination, "a");
+
+ // create work unit state
+ WorkUnitState state = createWorkUnitState(stagingDir, outputDir, cf);
+ CopyableDatasetMetadata metadata = new CopyableDatasetMetadata(new TestCopyableDataset(new Path("/source")));
+ CopySource.serializeCopyEntity(state, cf);
+ CopySource.serializeCopyableDataset(state, metadata);
+ // create writer
+ FileAwareInputStreamDataWriter writer = new FileAwareInputStreamDataWriter(state, fs,1, 0, null);
+
+ // create output of writer.write
+ Path writtenFile = writer.getStagingFilePath(cf);
+ this.fs.mkdirs(writtenFile.getParent());
+ this.fs.createNewFile(writtenFile);
+ Path outputRoot = FileAwareInputStreamDataWriter.getPartitionOutputRoot(outputDir, cf.getDatasetAndPartition(metadata));
+ Path expectedOutputPath = new Path(outputRoot, destinationWithoutLeadingSeparator);
+ // check ancestor path is absent before commit
+ Assert.assertFalse(this.fs.exists(expectedOutputPath.getParent().getParent()));
+ writer.actualProcessedCopyableFile = Optional.of(cf);
+ // commit
+ writer.commit();
+ // check ancestor path was created as part of commit
+ Assert.assertTrue(this.fs.exists(expectedOutputPath.getParent().getParent()));
+ verifyAclEntries(writer, this.fs.getPathToAclEntries(), expectedOutputPath, outputDir);
+ }
+
+ @Test
+ public void testCommitWithAclPreservationWhenAncestorPathsPresent() throws IOException {
+ String fileName = "file";
+ // Asemble destination paths
+ String destinationExistingToken = "destination";
+ String destinationAdditionalTokens = "path";
+ Path destination = new Path(new Path(new Path("/", destinationExistingToken), destinationAdditionalTokens), fileName);
+ Path destinationWithoutLeadingSeparator = new Path(new Path(destinationExistingToken, destinationAdditionalTokens), fileName);
+
+ // Create temp directory
+ File tmpFile = Files.createTempDir();
+ tmpFile.deleteOnExit();
+ Path tmpPath = new Path(tmpFile.getAbsolutePath());
+
+ // create origin file
+ Path originFile = new Path(tmpPath, fileName);
+ this.fs.createNewFile(originFile);
+
+ // create stating dir
+ Path stagingDir = new Path(tmpPath, "staging");
+ this.fs.mkdirs(stagingDir);
+
+ // create output dir
+ Path outputDir = new Path(tmpPath, "output");
+ this.fs.mkdirs(outputDir);
+
+ // create copyable file
+ CopyableFile cf = createCopyableFile(originFile, destination, "a");
+
+ // create work unit state
+ WorkUnitState state = createWorkUnitState(stagingDir, outputDir, cf);
+ CopyableDatasetMetadata metadata = new CopyableDatasetMetadata(new TestCopyableDataset(new Path("/source")));
+ CopySource.serializeCopyEntity(state, cf);
+ CopySource.serializeCopyableDataset(state, metadata);
+ // create writer
+ FileAwareInputStreamDataWriter writer = new FileAwareInputStreamDataWriter(state, fs,1, 0, null);
+
+ // create output of writer.write
+ Path writtenFile = writer.getStagingFilePath(cf);
+ this.fs.mkdirs(writtenFile.getParent());
+ this.fs.createNewFile(writtenFile);
+ // create existing directories in writer output
+ Path outputRoot = FileAwareInputStreamDataWriter.getPartitionOutputRoot(outputDir, cf.getDatasetAndPartition(metadata));
+ Path existingOutputPath = new Path(outputRoot, destinationExistingToken);
+ Path expectedOutputPath = new Path(outputRoot, destinationWithoutLeadingSeparator);
+ // create output path and check ancestor path is present
+ this.fs.mkdirs(existingOutputPath);
+ Assert.assertTrue(this.fs.exists(expectedOutputPath.getParent().getParent()));
+ writer.actualProcessedCopyableFile = Optional.of(cf);
+ // commit
+ writer.commit();
+ verifyAclEntries(writer, this.fs.getPathToAclEntries(), expectedOutputPath, existingOutputPath);
+ }
+
+ private void verifyAclEntries(FileAwareInputStreamDataWriter writer, ImmutableMap pathToAclEntries, Path expectedOutputPath, Path ancestorRoot) {
+ // fetching and preparing file paths from FileAwareInputStreamDataWriter object
+ Path outputDir = writer.outputDir;
+ String[] splitExpectedOutputPath = expectedOutputPath.toString().split("output");
+ Path dstOutputPath = new Path(outputDir.toString().concat(splitExpectedOutputPath[1])).getParent();
+
+ OwnerAndPermission destinationOwnerAndPermission = writer.actualProcessedCopyableFile.get().getDestinationOwnerAndPermission();
+ List<AclEntry> actual = destinationOwnerAndPermission.getAclEntries();
+ while (!dstOutputPath.equals(ancestorRoot)) {
+ List<AclEntry> expected = (List<AclEntry>) pathToAclEntries.get(dstOutputPath);
+ Assert.assertEquals(actual, expected);
+ dstOutputPath = dstOutputPath.getParent();
+ }
+ }
+
+ private WorkUnitState createWorkUnitState(Path stagingDir, Path outputDir, CopyableFile cf) {
Review Comment:
thanks Kip! And I get your point about the test cases being similar... and that's what I was trying to do and see if common code could be reused... but then started wondering if I can generalize these abstractions for all the unit tests in this class and it was time consuming.... but yes, we can come back and refactor it!
--
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
[GitHub] [gobblin] meethngala commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by "meethngala (via GitHub)" <gi...@apache.org>.
meethngala commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1099619637
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -523,11 +509,14 @@ public void cleanup() {
}
}
- protected static class TestLocalFileSystem extends LocalFileSystem {
-
+ protected class TestLocalFileSystem extends LocalFileSystem {
Review Comment:
added the documentation in my latest PR!
--
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
[GitHub] [gobblin] phet commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1097909721
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -523,11 +509,14 @@ public void cleanup() {
}
}
- protected static class TestLocalFileSystem extends LocalFileSystem {
-
+ protected class TestLocalFileSystem extends LocalFileSystem {
+ ConcurrentHashMap<Path, List<AclEntry>> pathToAclEntries = new ConcurrentHashMap<>();
@Override
public void setAcl(Path path, List<AclEntry> aclEntries) {
pathToAclEntries.put(path, aclEntries);
}
+ public ConcurrentHashMap<Path, List<AclEntry>> getPathToAclEntries() {
+ return pathToAclEntries;
Review Comment:
first off, I recognize this is just a unit test... but still I'd look for it to reach at least somewhere on this spectrum:
1. the return type shouldn't be `ConcurrentHashMap`--because the caller's not supposed to write to it!
2. return `ImmutableMap` so the caller breaks on any misguided attempt to write to it (even though not isolated from concurrent puts)
3. best practice of a defensive copy by returning a new map (to freeze it at a point in time). here, should be `synchronized` to avoid concurrent modification exception during iterateration
--
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
[GitHub] [gobblin] phet commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1099045862
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -402,10 +409,169 @@ public void testCommit() throws IOException {
// previously existing paths should not have permissions changed
fileStatus = this.fs.getFileStatus(existingOutputPath);
Assert.assertEquals(fileStatus.getPermission(), existingPathPermission);
-
Assert.assertFalse(this.fs.exists(writer.stagingDir));
}
+ @Test
+ public void testCommitWithAclPreservationWhenAncestorPathsAbsent() throws IOException {
+ String fileName = "file";
+ // Asemble destination paths
+ String destinationExistingToken = "destination";
+ String destinationAdditionalTokens = "path";
+ Path destination = new Path(new Path(new Path("/", destinationExistingToken), destinationAdditionalTokens), fileName);
+ Path destinationWithoutLeadingSeparator = new Path(new Path(destinationExistingToken, destinationAdditionalTokens), fileName);
+
+ // Create temp directory
+ File tmpFile = Files.createTempDir();
+ tmpFile.deleteOnExit();
+ Path tmpPath = new Path(tmpFile.getAbsolutePath());
+
+ // create origin file
+ Path originFile = new Path(tmpPath, fileName);
+ this.fs.createNewFile(originFile);
+
+ // create stating dir
+ Path stagingDir = new Path(tmpPath, "staging");
+ this.fs.mkdirs(stagingDir);
+
+ // create output dir
+ Path outputDir = new Path(tmpPath, "output");
+ this.fs.mkdirs(outputDir);
+
+ // create copyable file
+ CopyableFile cf = createCopyableFile(originFile, destination, "a");
+
+ // create work unit state
+ WorkUnitState state = createWorkUnitState(stagingDir, outputDir, cf);
+ CopyableDatasetMetadata metadata = new CopyableDatasetMetadata(new TestCopyableDataset(new Path("/source")));
+ CopySource.serializeCopyEntity(state, cf);
+ CopySource.serializeCopyableDataset(state, metadata);
+ // create writer
+ FileAwareInputStreamDataWriter writer = new FileAwareInputStreamDataWriter(state, fs,1, 0, null);
+
+ // create output of writer.write
+ Path writtenFile = writer.getStagingFilePath(cf);
+ this.fs.mkdirs(writtenFile.getParent());
+ this.fs.createNewFile(writtenFile);
+ Path outputRoot = FileAwareInputStreamDataWriter.getPartitionOutputRoot(outputDir, cf.getDatasetAndPartition(metadata));
+ Path expectedOutputPath = new Path(outputRoot, destinationWithoutLeadingSeparator);
+ // check ancestor path is absent before commit
+ Assert.assertFalse(this.fs.exists(expectedOutputPath.getParent().getParent()));
+ writer.actualProcessedCopyableFile = Optional.of(cf);
+ // commit
+ writer.commit();
+ // check ancestor path was created as part of commit
+ Assert.assertTrue(this.fs.exists(expectedOutputPath.getParent().getParent()));
+ verifyAclEntries(writer, this.fs.getPathToAclEntries(), expectedOutputPath, outputDir);
+ }
+
+ @Test
+ public void testCommitWithAclPreservationWhenAncestorPathsPresent() throws IOException {
+ String fileName = "file";
+ // Asemble destination paths
+ String destinationExistingToken = "destination";
+ String destinationAdditionalTokens = "path";
+ Path destination = new Path(new Path(new Path("/", destinationExistingToken), destinationAdditionalTokens), fileName);
+ Path destinationWithoutLeadingSeparator = new Path(new Path(destinationExistingToken, destinationAdditionalTokens), fileName);
+
+ // Create temp directory
+ File tmpFile = Files.createTempDir();
+ tmpFile.deleteOnExit();
+ Path tmpPath = new Path(tmpFile.getAbsolutePath());
+
+ // create origin file
+ Path originFile = new Path(tmpPath, fileName);
+ this.fs.createNewFile(originFile);
+
+ // create stating dir
+ Path stagingDir = new Path(tmpPath, "staging");
+ this.fs.mkdirs(stagingDir);
+
+ // create output dir
+ Path outputDir = new Path(tmpPath, "output");
+ this.fs.mkdirs(outputDir);
+
+ // create copyable file
+ CopyableFile cf = createCopyableFile(originFile, destination, "a");
+
+ // create work unit state
+ WorkUnitState state = createWorkUnitState(stagingDir, outputDir, cf);
+ CopyableDatasetMetadata metadata = new CopyableDatasetMetadata(new TestCopyableDataset(new Path("/source")));
+ CopySource.serializeCopyEntity(state, cf);
+ CopySource.serializeCopyableDataset(state, metadata);
+ // create writer
+ FileAwareInputStreamDataWriter writer = new FileAwareInputStreamDataWriter(state, fs,1, 0, null);
+
+ // create output of writer.write
+ Path writtenFile = writer.getStagingFilePath(cf);
+ this.fs.mkdirs(writtenFile.getParent());
+ this.fs.createNewFile(writtenFile);
+ // create existing directories in writer output
+ Path outputRoot = FileAwareInputStreamDataWriter.getPartitionOutputRoot(outputDir, cf.getDatasetAndPartition(metadata));
+ Path existingOutputPath = new Path(outputRoot, destinationExistingToken);
+ Path expectedOutputPath = new Path(outputRoot, destinationWithoutLeadingSeparator);
+ // create output path and check ancestor path is present
+ this.fs.mkdirs(existingOutputPath);
+ Assert.assertTrue(this.fs.exists(expectedOutputPath.getParent().getParent()));
+ writer.actualProcessedCopyableFile = Optional.of(cf);
+ // commit
+ writer.commit();
+ verifyAclEntries(writer, this.fs.getPathToAclEntries(), expectedOutputPath, existingOutputPath);
+ }
+
+ private void verifyAclEntries(FileAwareInputStreamDataWriter writer, ImmutableMap pathToAclEntries, Path expectedOutputPath, Path ancestorRoot) {
+ // fetching and preparing file paths from FileAwareInputStreamDataWriter object
+ Path outputDir = writer.outputDir;
+ String[] splitExpectedOutputPath = expectedOutputPath.toString().split("output");
+ Path dstOutputPath = new Path(outputDir.toString().concat(splitExpectedOutputPath[1])).getParent();
+
+ OwnerAndPermission destinationOwnerAndPermission = writer.actualProcessedCopyableFile.get().getDestinationOwnerAndPermission();
+ List<AclEntry> actual = destinationOwnerAndPermission.getAclEntries();
+ while (!dstOutputPath.equals(ancestorRoot)) {
+ List<AclEntry> expected = (List<AclEntry>) pathToAclEntries.get(dstOutputPath);
+ Assert.assertEquals(actual, expected);
+ dstOutputPath = dstOutputPath.getParent();
+ }
+ }
+
+ private WorkUnitState createWorkUnitState(Path stagingDir, Path outputDir, CopyableFile cf) {
Review Comment:
nice abstractions, this and `createCopyableFile`
--
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
[GitHub] [gobblin] phet commented on a diff in pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1099056706
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -402,10 +409,169 @@ public void testCommit() throws IOException {
// previously existing paths should not have permissions changed
fileStatus = this.fs.getFileStatus(existingOutputPath);
Assert.assertEquals(fileStatus.getPermission(), existingPathPermission);
-
Assert.assertFalse(this.fs.exists(writer.stagingDir));
}
+ @Test
+ public void testCommitWithAclPreservationWhenAncestorPathsAbsent() throws IOException {
+ String fileName = "file";
+ // Asemble destination paths
+ String destinationExistingToken = "destination";
+ String destinationAdditionalTokens = "path";
+ Path destination = new Path(new Path(new Path("/", destinationExistingToken), destinationAdditionalTokens), fileName);
+ Path destinationWithoutLeadingSeparator = new Path(new Path(destinationExistingToken, destinationAdditionalTokens), fileName);
Review Comment:
aren't these two equivalent:
```
Path destWoLS = ...
Path destination = new Path("/", destWoLS);
```
?
--
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
[GitHub] [gobblin] codecov-commenter commented on pull request #3616: [GOBBLIN-1755] Support extended ACLs and sticky bit for file based distcp
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#issuecomment-1350474765
# [Codecov](https://codecov.io/gh/apache/gobblin/pull/3616?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#3616](https://codecov.io/gh/apache/gobblin/pull/3616?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a176b0e) into [master](https://codecov.io/gh/apache/gobblin/commit/d29bd9f32158bbc2cc9c9bbbdb90d565fb34b078?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d29bd9f) will **decrease** coverage by `3.18%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #3616 +/- ##
============================================
- Coverage 46.91% 43.72% -3.19%
+ Complexity 10699 2060 -8639
============================================
Files 2128 408 -1720
Lines 83231 17618 -65613
Branches 9275 2151 -7124
============================================
- Hits 39044 7704 -31340
+ Misses 40598 9056 -31542
+ Partials 3589 858 -2731
```
| [Impacted Files](https://codecov.io/gh/apache/gobblin/pull/3616?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...a/org/apache/gobblin/util/limiter/NoopLimiter.java](https://codecov.io/gh/apache/gobblin/pull/3616/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvbGltaXRlci9Ob29wTGltaXRlci5qYXZh) | `40.00% <0.00%> (-20.00%)` | :arrow_down: |
| [...a/org/apache/gobblin/cluster/GobblinHelixTask.java](https://codecov.io/gh/apache/gobblin/pull/3616/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvR29iYmxpbkhlbGl4VGFzay5qYXZh) | `60.21% <0.00%> (-4.31%)` | :arrow_down: |
| [...lin/util/filesystem/FileSystemInstrumentation.java](https://codecov.io/gh/apache/gobblin/pull/3616/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvZmlsZXN5c3RlbS9GaWxlU3lzdGVtSW5zdHJ1bWVudGF0aW9uLmphdmE=) | `92.85% <0.00%> (ø)` | |
| [...che/gobblin/data/management/copy/CopyableFile.java](https://codecov.io/gh/apache/gobblin/pull/3616/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L2NvcHkvQ29weWFibGVGaWxlLmphdmE=) | | |
| [...bblin/data/management/copy/OwnerAndPermission.java](https://codecov.io/gh/apache/gobblin/pull/3616/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L2NvcHkvT3duZXJBbmRQZXJtaXNzaW9uLmphdmE=) | | |
| [...bblin/data/management/copy/PreserveAttributes.java](https://codecov.io/gh/apache/gobblin/pull/3616/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L2NvcHkvUHJlc2VydmVBdHRyaWJ1dGVzLmphdmE=) | | |
| [...nt/copy/writer/FileAwareInputStreamDataWriter.java](https://codecov.io/gh/apache/gobblin/pull/3616/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L2NvcHkvd3JpdGVyL0ZpbGVBd2FyZUlucHV0U3RyZWFtRGF0YVdyaXRlci5qYXZh) | | |
| [...ain/java/org/apache/gobblin/runtime/fork/Fork.java](https://codecov.io/gh/apache/gobblin/pull/3616/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvZm9yay9Gb3JrLmphdmE=) | | |
| [...runtime/messaging/handler/SplitMessageHandler.java](https://codecov.io/gh/apache/gobblin/pull/3616/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvbWVzc2FnaW5nL2hhbmRsZXIvU3BsaXRNZXNzYWdlSGFuZGxlci5qYXZh) | | |
| [...bblin/converter/StringFieldEncryptorConverter.java](https://codecov.io/gh/apache/gobblin/pull/3616/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4tY3J5cHRvLXByb3ZpZGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbnZlcnRlci9TdHJpbmdGaWVsZEVuY3J5cHRvckNvbnZlcnRlci5qYXZh) | | |
| ... and [1713 more](https://codecov.io/gh/apache/gobblin/pull/3616/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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