[GitHub] [hadoop] ayushtkn commented on a change in pull request #3987: HADOOP-18122. ViewFileSystem fails on determining owning group when primary group doesn't exist for user

[GitBox <gi...@apache.org>]

ayushtkn commented on a change in pull request #3987:
URL: https://github.com/apache/hadoop/pull/3987#discussion_r804901362



##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java
##########
@@ -1778,15 +1793,34 @@ public BlockStoragePolicySpi getStoragePolicy(Path src) throws IOException {
       Collection<BlockStoragePolicySpi> allPolicies = new HashSet<>();
       for (FileSystem fs : getChildFileSystems()) {
         try {
-          Collection<? extends BlockStoragePolicySpi> policies =
-              fs.getAllStoragePolicies();
+          Collection<? extends BlockStoragePolicySpi> policies = fs.getAllStoragePolicies();

Review comment:
       unrelated change

##########
File path: hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/ViewFileSystemBaseTest.java
##########
@@ -1479,16 +1484,12 @@ public void testTargetFileSystemLazyInitializationForChecksumMethods()
     final String clusterName = "cluster" + new Random().nextInt();
     Configuration config = new Configuration(conf);
     config.setBoolean(CONFIG_VIEWFS_ENABLE_INNER_CACHE, false);
-    config.setClass("fs.othermockfs.impl",
-        TestChRootedFileSystem.MockFileSystem.class, FileSystem.class);
-    ConfigUtil.addLink(config, clusterName, "/user",
-        URI.create("othermockfs://mockauth1/mockpath"));
-    ConfigUtil.addLink(config, clusterName,
-        "/mock", URI.create("othermockfs://mockauth/mockpath"));
+    config.setClass("fs.othermockfs.impl", TestChRootedFileSystem.MockFileSystem.class, FileSystem.class);
+    ConfigUtil.addLink(config, clusterName, "/user", URI.create("othermockfs://mockauth1/mockpath"));
+    ConfigUtil.addLink(config, clusterName, "/mock", URI.create("othermockfs://mockauth/mockpath"));

Review comment:
       looks like just formatting change, can you please remove the just formatting changes, here and other places as well.
   We should restrict ourselves to only related changes,

##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java
##########
@@ -1778,15 +1793,34 @@ public BlockStoragePolicySpi getStoragePolicy(Path src) throws IOException {
       Collection<BlockStoragePolicySpi> allPolicies = new HashSet<>();
       for (FileSystem fs : getChildFileSystems()) {
         try {
-          Collection<? extends BlockStoragePolicySpi> policies =
-              fs.getAllStoragePolicies();
+          Collection<? extends BlockStoragePolicySpi> policies = fs.getAllStoragePolicies();
           allPolicies.addAll(policies);
         } catch (UnsupportedOperationException e) {
           // ignored
         }
       }
       return allPolicies;
     }
+
+    private FsPermission getMountLinkDefaultPermissions() {
+      return PERMISSION_555;
+    }
+
+    private String getMountLinkUserName() {
+      String username = config.get(CONFIG_VIEWFS_MOUNT_LINKS_USER_NAME);

Review comment:
       we would be fetching the value everytime from the config for every operation? why not get once and then store it. the config object won't change post FS has been initialised?

##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java
##########
@@ -1682,9 +1696,10 @@ public void setAcl(Path path, List<AclEntry> aclSpec) throws IOException {
     @Override
     public AclStatus getAclStatus(Path path) throws IOException {
       checkPathIsSlash(path);
-      return new AclStatus.Builder().owner(ugi.getShortUserName())
-          .group(ugi.getPrimaryGroupName())
-          .addEntries(AclUtil.getMinimalAcl(PERMISSION_555))
+      return new AclStatus.Builder().owner(getMountLinkUserName())
+          .group(getMountLinkGroupName())
+          .setPermission(PERMISSION_555)

Review comment:
       why not getMountLinkDefaultPermissions

##########
File path: hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/ViewFileSystemBaseTest.java
##########
@@ -39,6 +39,11 @@
 import org.apache.hadoop.fs.FileStatus;
 import org.apache.hadoop.fs.FileSystem;
 import org.apache.hadoop.fs.FileSystemTestHelper;
+
+import static org.apache.hadoop.fs.viewfs.Constants.CONFIG_VIEWFS_MOUNT_LINKS_USER_NAME;
+import static org.apache.hadoop.fs.viewfs.Constants.CONFIG_VIEWFS_MOUNT_LINKS_GROUP_NAME;
+import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.spy;

Review comment:
       import order is wrong, should be with other static imports




-- 
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: common-issues-unsubscribe@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org