From dd7916d3cd5d880d0b257d229f43f10feff04c93 Mon Sep 17 00:00:00 2001 From: Andrew Wang Date: Thu, 17 Aug 2017 09:35:36 -0700 Subject: [PATCH] HDFS-12250. Reduce usage of FsPermissionExtension in unit tests. Contributed by Chris Douglas. --- .../hadoop/fs/permission/FsPermission.java | 2 +- .../apache/hadoop/fs/shell/AclCommands.java | 6 ++--- .../fs/shell/CommandWithDestination.java | 4 ++-- .../java/org/apache/hadoop/fs/shell/Ls.java | 4 ++-- .../fs/http/client/BaseTestHttpFSWith.java | 1 + .../org/apache/hadoop/hdfs/TestDFSShell.java | 24 +++++++++---------- .../hdfs/server/namenode/FSAclBaseTest.java | 6 +++++ .../ClientDistributedCacheManager.java | 6 ++--- .../hadoop/fs/adl/TestGetFileStatus.java | 1 + .../hadoop/tools/CopyListingFileStatus.java | 4 ++-- .../apache/hadoop/tools/util/DistCpUtils.java | 4 +--- 11 files changed, 33 insertions(+), 29 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/permission/FsPermission.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/permission/FsPermission.java index 23692de732..031092bbff 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/permission/FsPermission.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/permission/FsPermission.java @@ -163,7 +163,7 @@ public FsPermission getUnmasked() { */ public static FsPermission read(DataInput in) throws IOException { FsPermission p = new FsPermission(); - p.readFields(in); + p.fromShort(in.readShort()); return p; } diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/AclCommands.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/AclCommands.java index a5e386c785..701c9deb9c 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/AclCommands.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/AclCommands.java @@ -86,9 +86,9 @@ protected void processPath(PathData item) throws IOException { (perm.getOtherAction().implies(FsAction.EXECUTE) ? "t" : "T")); } - AclStatus aclStatus = null; - List entries = null; - if (perm.getAclBit()) { + final AclStatus aclStatus; + final List entries; + if (item.stat.hasAcl()) { aclStatus = item.fs.getAclStatus(item.path); entries = aclStatus.getEntries(); } else { diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CommandWithDestination.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CommandWithDestination.java index 2a483c0a50..0bd48827f2 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CommandWithDestination.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CommandWithDestination.java @@ -444,8 +444,8 @@ protected void preserveAttributes(PathData src, PathData target, src.stat.getPermission()); } if (shouldPreserve(FileAttribute.ACL)) { - FsPermission perm = src.stat.getPermission(); - if (perm.getAclBit()) { + if (src.stat.hasAcl()) { + FsPermission perm = src.stat.getPermission(); List srcEntries = src.fs.getAclStatus(src.path).getEntries(); List srcFullEntries = diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Ls.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Ls.java index 221b3cb3d3..a2d5017f6f 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Ls.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Ls.java @@ -255,7 +255,7 @@ protected void processPath(PathData item) throws IOException { ContentSummary contentSummary = item.fs.getContentSummary(item.path); String line = String.format(lineFormat, (stat.isDirectory() ? "d" : "-"), - stat.getPermission() + (stat.getPermission().getAclBit() ? "+" : " "), + stat.getPermission() + (stat.hasAcl() ? "+" : " "), (stat.isFile() ? stat.getReplication() : "-"), stat.getOwner(), stat.getGroup(), @@ -269,7 +269,7 @@ protected void processPath(PathData item) throws IOException { } else { String line = String.format(lineFormat, (stat.isDirectory() ? "d" : "-"), - stat.getPermission() + (stat.getPermission().getAclBit() ? "+" : " "), + stat.getPermission() + (stat.hasAcl() ? "+" : " "), (stat.isFile() ? stat.getReplication() : "-"), stat.getOwner(), stat.getGroup(), diff --git a/hadoop-hdfs-project/hadoop-hdfs-httpfs/src/test/java/org/apache/hadoop/fs/http/client/BaseTestHttpFSWith.java b/hadoop-hdfs-project/hadoop-hdfs-httpfs/src/test/java/org/apache/hadoop/fs/http/client/BaseTestHttpFSWith.java index 553bbcebf5..2cd89344aa 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-httpfs/src/test/java/org/apache/hadoop/fs/http/client/BaseTestHttpFSWith.java +++ b/hadoop-hdfs-project/hadoop-hdfs-httpfs/src/test/java/org/apache/hadoop/fs/http/client/BaseTestHttpFSWith.java @@ -859,6 +859,7 @@ private static void assertSameAcls(FileSystem expected, FileSystem actual, FileStatus expectedFileStatus = expected.getFileStatus(path); FileStatus actualFileStatus = actual.getFileStatus(path); assertEquals(actualFileStatus.hasAcl(), expectedFileStatus.hasAcl()); + // backwards compat assertEquals(actualFileStatus.getPermission().getAclBit(), expectedFileStatus.getPermission().getAclBit()); } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSShell.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSShell.java index 27d41b4d32..9ae49aa9ef 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSShell.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSShell.java @@ -2189,7 +2189,7 @@ public void testCopyCommandsWithPreserveOption() throws Exception { assertTrue(xattrs.isEmpty()); List acls = dfs.getAclStatus(target1).getEntries(); assertTrue(acls.isEmpty()); - assertFalse(targetPerm.getAclBit()); + assertFalse(targetStatus.hasAcl()); // -ptop Path target2 = new Path(hdfsTestDir, "targetfile2"); @@ -2208,7 +2208,7 @@ public void testCopyCommandsWithPreserveOption() throws Exception { assertTrue(xattrs.isEmpty()); acls = dfs.getAclStatus(target2).getEntries(); assertTrue(acls.isEmpty()); - assertFalse(targetPerm.getAclBit()); + assertFalse(targetStatus.hasAcl()); // -ptopx Path target3 = new Path(hdfsTestDir, "targetfile3"); @@ -2229,7 +2229,7 @@ public void testCopyCommandsWithPreserveOption() throws Exception { assertArrayEquals(TRUSTED_A1_VALUE, xattrs.get(TRUSTED_A1)); acls = dfs.getAclStatus(target3).getEntries(); assertTrue(acls.isEmpty()); - assertFalse(targetPerm.getAclBit()); + assertFalse(targetStatus.hasAcl()); // -ptopa Path target4 = new Path(hdfsTestDir, "targetfile4"); @@ -2248,7 +2248,7 @@ public void testCopyCommandsWithPreserveOption() throws Exception { assertTrue(xattrs.isEmpty()); acls = dfs.getAclStatus(target4).getEntries(); assertFalse(acls.isEmpty()); - assertTrue(targetPerm.getAclBit()); + assertTrue(targetStatus.hasAcl()); assertEquals(dfs.getAclStatus(src), dfs.getAclStatus(target4)); // -ptoa (verify -pa option will preserve permissions also) @@ -2268,7 +2268,7 @@ public void testCopyCommandsWithPreserveOption() throws Exception { assertTrue(xattrs.isEmpty()); acls = dfs.getAclStatus(target5).getEntries(); assertFalse(acls.isEmpty()); - assertTrue(targetPerm.getAclBit()); + assertTrue(targetStatus.hasAcl()); assertEquals(dfs.getAclStatus(src), dfs.getAclStatus(target5)); } finally { if (null != shell) { @@ -2480,7 +2480,7 @@ public void testCopyCommandsToDirectoryWithPreserveOption() assertTrue(xattrs.isEmpty()); List acls = dfs.getAclStatus(targetDir1).getEntries(); assertTrue(acls.isEmpty()); - assertFalse(targetPerm.getAclBit()); + assertFalse(targetStatus.hasAcl()); // -ptop Path targetDir2 = new Path(hdfsTestDir, "targetDir2"); @@ -2499,7 +2499,7 @@ public void testCopyCommandsToDirectoryWithPreserveOption() assertTrue(xattrs.isEmpty()); acls = dfs.getAclStatus(targetDir2).getEntries(); assertTrue(acls.isEmpty()); - assertFalse(targetPerm.getAclBit()); + assertFalse(targetStatus.hasAcl()); // -ptopx Path targetDir3 = new Path(hdfsTestDir, "targetDir3"); @@ -2520,7 +2520,7 @@ public void testCopyCommandsToDirectoryWithPreserveOption() assertArrayEquals(TRUSTED_A1_VALUE, xattrs.get(TRUSTED_A1)); acls = dfs.getAclStatus(targetDir3).getEntries(); assertTrue(acls.isEmpty()); - assertFalse(targetPerm.getAclBit()); + assertFalse(targetStatus.hasAcl()); // -ptopa Path targetDir4 = new Path(hdfsTestDir, "targetDir4"); @@ -2539,7 +2539,7 @@ public void testCopyCommandsToDirectoryWithPreserveOption() assertTrue(xattrs.isEmpty()); acls = dfs.getAclStatus(targetDir4).getEntries(); assertFalse(acls.isEmpty()); - assertTrue(targetPerm.getAclBit()); + assertTrue(targetStatus.hasAcl()); assertEquals(dfs.getAclStatus(srcDir), dfs.getAclStatus(targetDir4)); // -ptoa (verify -pa option will preserve permissions also) @@ -2559,7 +2559,7 @@ public void testCopyCommandsToDirectoryWithPreserveOption() assertTrue(xattrs.isEmpty()); acls = dfs.getAclStatus(targetDir5).getEntries(); assertFalse(acls.isEmpty()); - assertTrue(targetPerm.getAclBit()); + assertTrue(targetStatus.hasAcl()); assertEquals(dfs.getAclStatus(srcDir), dfs.getAclStatus(targetDir5)); } finally { if (shell != null) { @@ -2615,7 +2615,7 @@ public void testCopyCommandsPreserveAclAndStickyBit() throws Exception { assertTrue(perm.equals(targetPerm)); List acls = dfs.getAclStatus(target1).getEntries(); assertTrue(acls.isEmpty()); - assertFalse(targetPerm.getAclBit()); + assertFalse(targetStatus.hasAcl()); // -ptopa preserves both sticky bit and ACL Path target2 = new Path(hdfsTestDir, "targetfile2"); @@ -2632,7 +2632,7 @@ public void testCopyCommandsPreserveAclAndStickyBit() throws Exception { assertTrue(perm.equals(targetPerm)); acls = dfs.getAclStatus(target2).getEntries(); assertFalse(acls.isEmpty()); - assertTrue(targetPerm.getAclBit()); + assertTrue(targetStatus.hasAcl()); assertEquals(dfs.getAclStatus(src), dfs.getAclStatus(target2)); } finally { if (null != shell) { diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/FSAclBaseTest.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/FSAclBaseTest.java index 93a83fd3dc..ee92217626 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/FSAclBaseTest.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/FSAclBaseTest.java @@ -32,6 +32,7 @@ import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.CommonConfigurationKeys; +import org.apache.hadoop.fs.FileStatus; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; import org.apache.hadoop.fs.permission.AclEntry; @@ -886,6 +887,11 @@ public void testSetPermissionCannotSetAclBit() throws IOException { FsPermission perm = inode.getFsPermission(); assertNotNull(perm); assertEquals(0755, perm.toShort()); + FileStatus stat = fs.getFileStatus(path); + assertFalse(stat.hasAcl()); + assertFalse(stat.isEncrypted()); + assertFalse(stat.isErasureCoded()); + // backwards-compat check assertEquals(0755, perm.toExtendedShort()); assertAclFeature(false); } diff --git a/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/filecache/ClientDistributedCacheManager.java b/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/filecache/ClientDistributedCacheManager.java index 9f8edb5df0..ada14db944 100644 --- a/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/filecache/ClientDistributedCacheManager.java +++ b/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/filecache/ClientDistributedCacheManager.java @@ -28,7 +28,6 @@ import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; import org.apache.hadoop.fs.permission.FsAction; -import org.apache.hadoop.fs.permission.FsPermission; import org.apache.hadoop.mapreduce.MRJobConfig; import org.apache.hadoop.mapreduce.security.TokenCache; import org.apache.hadoop.security.Credentials; @@ -293,7 +292,6 @@ static boolean ancestorsHaveExecutePermissions(FileSystem fs, Path path, private static boolean checkPermissionOfOther(FileSystem fs, Path path, FsAction action, Map statCache) throws IOException { FileStatus status = getFileStatus(fs, path.toUri(), statCache); - FsPermission perms = status.getPermission(); // Encrypted files are always treated as private. This stance has two // important side effects. The first is that the encrypted files will be @@ -302,8 +300,8 @@ private static boolean checkPermissionOfOther(FileSystem fs, Path path, // world readable permissions that is stored in an encryption zone from // being localized as a publicly shared file with world readable // permissions. - if (!perms.getEncryptedBit()) { - FsAction otherAction = perms.getOtherAction(); + if (!status.isEncrypted()) { + FsAction otherAction = status.getPermission().getOtherAction(); if (otherAction.implies(action)) { return true; } diff --git a/hadoop-tools/hadoop-azure-datalake/src/test/java/org/apache/hadoop/fs/adl/TestGetFileStatus.java b/hadoop-tools/hadoop-azure-datalake/src/test/java/org/apache/hadoop/fs/adl/TestGetFileStatus.java index d9e22db9cd..95c2363956 100644 --- a/hadoop-tools/hadoop-azure-datalake/src/test/java/org/apache/hadoop/fs/adl/TestGetFileStatus.java +++ b/hadoop-tools/hadoop-azure-datalake/src/test/java/org/apache/hadoop/fs/adl/TestGetFileStatus.java @@ -98,4 +98,5 @@ public void getFileStatusAclBit() throws URISyntaxException, IOException { Assert.assertFalse(fileStatus.hasAcl()); Assert.assertFalse(fileStatus.getPermission().getAclBit()); } + } diff --git a/hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/CopyListingFileStatus.java b/hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/CopyListingFileStatus.java index 29c59ac103..138b491189 100644 --- a/hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/CopyListingFileStatus.java +++ b/hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/CopyListingFileStatus.java @@ -280,7 +280,7 @@ public void write(DataOutput out) throws IOException { out.writeLong(getBlockSize()); out.writeLong(getModificationTime()); out.writeLong(getAccessTime()); - getPermission().write(out); + out.writeShort(getPermission().toShort()); Text.writeString(out, getOwner(), Text.DEFAULT_MAX_LEN); Text.writeString(out, getGroup(), Text.DEFAULT_MAX_LEN); if (aclEntries != null) { @@ -330,7 +330,7 @@ public void readFields(DataInput in) throws IOException { blocksize = in.readLong(); modificationTime = in.readLong(); accessTime = in.readLong(); - permission.readFields(in); + permission.fromShort(in.readShort()); owner = Text.readString(in, Text.DEFAULT_MAX_LEN); group = Text.readString(in, Text.DEFAULT_MAX_LEN); byte aclEntriesSize = in.readByte(); diff --git a/hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/util/DistCpUtils.java b/hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/util/DistCpUtils.java index dbe750a65c..2b3b529994 100644 --- a/hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/util/DistCpUtils.java +++ b/hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/util/DistCpUtils.java @@ -31,7 +31,6 @@ import org.apache.hadoop.fs.XAttr; import org.apache.hadoop.fs.permission.AclEntry; import org.apache.hadoop.fs.permission.AclUtil; -import org.apache.hadoop.fs.permission.FsPermission; import org.apache.hadoop.hdfs.DistributedFileSystem; import org.apache.hadoop.io.SequenceFile; import org.apache.hadoop.io.Text; @@ -403,8 +402,7 @@ public static CopyListingFileStatus toCopyListingFileStatusHelper( CopyListingFileStatus copyListingFileStatus = new CopyListingFileStatus(fileStatus, chunkOffset, chunkLength); if (preserveAcls) { - FsPermission perm = fileStatus.getPermission(); - if (perm.getAclBit()) { + if (fileStatus.hasAcl()) { List aclEntries = fileSystem.getAclStatus( fileStatus.getPath()).getEntries(); copyListingFileStatus.setAclEntries(aclEntries);