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

["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