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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
