HADOOP-13442. Optimize UGI group lookups. Contributed by Daryn Sharp.
Project: http://git-wip-us.apache.org/repos/asf/hadoop/repo Commit: http://git-wip-us.apache.org/repos/asf/hadoop/commit/94225152 Tree: http://git-wip-us.apache.org/repos/asf/hadoop/tree/94225152 Diff: http://git-wip-us.apache.org/repos/asf/hadoop/diff/94225152 Branch: refs/heads/YARN-2915 Commit: 94225152399e6e89fa7b4cff6d17d33e544329a3 Parents: 6ae3919 Author: Kihwal Lee <kih...@apache.org> Authored: Thu Aug 4 10:45:55 2016 -0500 Committer: Kihwal Lee <kih...@apache.org> Committed: Thu Aug 4 10:45:55 2016 -0500 ---------------------------------------------------------------------- .../java/org/apache/hadoop/fs/FileSystem.java | 4 +-- .../apache/hadoop/fs/viewfs/ViewFileSystem.java | 6 ++--- .../org/apache/hadoop/fs/viewfs/ViewFs.java | 12 ++++----- .../org/apache/hadoop/io/SecureIOUtils.java | 3 +-- .../java/org/apache/hadoop/security/Groups.java | 24 ++++++++++++----- .../hadoop/security/UserGroupInformation.java | 28 +++++++++++++------- .../security/authorize/AccessControlList.java | 2 +- .../security/TestUserGroupInformation.java | 6 +++-- 8 files changed, 52 insertions(+), 33 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/hadoop/blob/94225152/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java ---------------------------------------------------------------------- diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java index 146bce5..1fdfac0 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java @@ -26,7 +26,6 @@ import java.net.URI; import java.net.URISyntaxException; import java.security.PrivilegedExceptionAction; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collection; import java.util.EnumSet; import java.util.HashMap; @@ -2221,12 +2220,11 @@ public abstract class FileSystem extends Configured implements Closeable { FsPermission perm = stat.getPermission(); UserGroupInformation ugi = UserGroupInformation.getCurrentUser(); String user = ugi.getShortUserName(); - List<String> groups = Arrays.asList(ugi.getGroupNames()); if (user.equals(stat.getOwner())) { if (perm.getUserAction().implies(mode)) { return; } - } else if (groups.contains(stat.getGroup())) { + } else if (ugi.getGroups().contains(stat.getGroup())) { if (perm.getGroupAction().implies(mode)) { return; } http://git-wip-us.apache.org/repos/asf/hadoop/blob/94225152/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java ---------------------------------------------------------------------- diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java index f7047b7..edc59ab 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java @@ -901,7 +901,7 @@ public class ViewFileSystem extends FileSystem { public FileStatus getFileStatus(Path f) throws IOException { checkPathIsSlash(f); return new FileStatus(0, true, 0, 0, creationTime, creationTime, - PERMISSION_555, ugi.getUserName(), ugi.getGroupNames()[0], + PERMISSION_555, ugi.getUserName(), ugi.getPrimaryGroupName(), new Path(theInternalDir.fullPath).makeQualified( myUri, ROOT_PATH)); @@ -922,7 +922,7 @@ public class ViewFileSystem extends FileSystem { result[i++] = new FileStatus(0, false, 0, 0, creationTime, creationTime, PERMISSION_555, - ugi.getUserName(), ugi.getGroupNames()[0], + ugi.getUserName(), ugi.getPrimaryGroupName(), link.getTargetLink(), new Path(inode.fullPath).makeQualified( myUri, null)); @@ -1054,7 +1054,7 @@ public class ViewFileSystem extends FileSystem { public AclStatus getAclStatus(Path path) throws IOException { checkPathIsSlash(path); return new AclStatus.Builder().owner(ugi.getUserName()) - .group(ugi.getGroupNames()[0]) + .group(ugi.getPrimaryGroupName()) .addEntries(AclUtil.getMinimalAcl(PERMISSION_555)) .stickyBit(false).build(); } http://git-wip-us.apache.org/repos/asf/hadoop/blob/94225152/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFs.java ---------------------------------------------------------------------- diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFs.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFs.java index 2f93296..23465a6 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFs.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFs.java @@ -843,14 +843,14 @@ public class ViewFs extends AbstractFileSystem { public FileStatus getFileStatus(final Path f) throws IOException { checkPathIsSlash(f); return new FileStatus(0, true, 0, 0, creationTime, creationTime, - PERMISSION_555, ugi.getUserName(), ugi.getGroupNames()[0], + PERMISSION_555, ugi.getUserName(), ugi.getPrimaryGroupName(), new Path(theInternalDir.fullPath).makeQualified( myUri, null)); } @Override public FileStatus getFileLinkStatus(final Path f) - throws FileNotFoundException { + throws IOException { // look up i internalDirs children - ignore first Slash INode<AbstractFileSystem> inode = theInternalDir.children.get(f.toUri().toString().substring(1)); @@ -863,13 +863,13 @@ public class ViewFs extends AbstractFileSystem { INodeLink<AbstractFileSystem> inodelink = (INodeLink<AbstractFileSystem>) inode; result = new FileStatus(0, false, 0, 0, creationTime, creationTime, - PERMISSION_555, ugi.getUserName(), ugi.getGroupNames()[0], + PERMISSION_555, ugi.getUserName(), ugi.getPrimaryGroupName(), inodelink.getTargetLink(), new Path(inode.fullPath).makeQualified( myUri, null)); } else { result = new FileStatus(0, true, 0, 0, creationTime, creationTime, - PERMISSION_555, ugi.getUserName(), ugi.getGroupNames()[0], + PERMISSION_555, ugi.getUserName(), ugi.getPrimaryGroupName(), new Path(inode.fullPath).makeQualified( myUri, null)); } @@ -908,7 +908,7 @@ public class ViewFs extends AbstractFileSystem { result[i++] = new FileStatus(0, false, 0, 0, creationTime, creationTime, - PERMISSION_555, ugi.getUserName(), ugi.getGroupNames()[0], + PERMISSION_555, ugi.getUserName(), ugi.getPrimaryGroupName(), link.getTargetLink(), new Path(inode.fullPath).makeQualified( myUri, null)); @@ -1042,7 +1042,7 @@ public class ViewFs extends AbstractFileSystem { public AclStatus getAclStatus(Path path) throws IOException { checkPathIsSlash(path); return new AclStatus.Builder().owner(ugi.getUserName()) - .group(ugi.getGroupNames()[0]) + .group(ugi.getPrimaryGroupName()) .addEntries(AclUtil.getMinimalAcl(PERMISSION_555)) .stickyBit(false).build(); } http://git-wip-us.apache.org/repos/asf/hadoop/blob/94225152/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/SecureIOUtils.java ---------------------------------------------------------------------- diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/SecureIOUtils.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/SecureIOUtils.java index 68333df..252ee4c 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/SecureIOUtils.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/SecureIOUtils.java @@ -22,7 +22,6 @@ import java.io.FileInputStream; import java.io.FileOutputStream; import java.io.IOException; import java.io.RandomAccessFile; -import java.util.Arrays; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.FSDataInputStream; @@ -273,7 +272,7 @@ public class SecureIOUtils { UserGroupInformation.createRemoteUser(expectedOwner); final String adminsGroupString = "Administrators"; success = owner.equals(adminsGroupString) - && Arrays.asList(ugi.getGroupNames()).contains(adminsGroupString); + && ugi.getGroups().contains(adminsGroupString); } else { success = false; } http://git-wip-us.apache.org/repos/asf/hadoop/blob/94225152/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Groups.java ---------------------------------------------------------------------- diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Groups.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Groups.java index 9a09c27..4891907 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Groups.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Groups.java @@ -18,9 +18,11 @@ package org.apache.hadoop.security; import java.io.IOException; +import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.HashMap; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -31,6 +33,7 @@ import java.util.concurrent.ThreadFactory; import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.atomic.AtomicReference; import org.apache.htrace.core.TraceScope; import org.apache.htrace.core.Tracer; @@ -74,8 +77,8 @@ public class Groups { private final GroupMappingServiceProvider impl; private final LoadingCache<String, List<String>> cache; - private final Map<String, List<String>> staticUserToGroupsMap = - new HashMap<String, List<String>>(); + private final AtomicReference<Map<String, List<String>>> staticMapRef = + new AtomicReference<>(); private final long cacheTimeout; private final long negativeCacheTimeout; private final long warningDeltaMs; @@ -164,6 +167,8 @@ public class Groups { CommonConfigurationKeys.HADOOP_USER_GROUP_STATIC_OVERRIDES_DEFAULT); Collection<String> mappings = StringUtils.getStringCollection( staticMapping, ";"); + Map<String, List<String>> staticUserToGroupsMap = + new HashMap<String, List<String>>(); for (String users : mappings) { Collection<String> userToGroups = StringUtils.getStringCollection(users, "="); @@ -182,6 +187,8 @@ public class Groups { } staticUserToGroupsMap.put(user, groups); } + staticMapRef.set( + staticUserToGroupsMap.isEmpty() ? null : staticUserToGroupsMap); } private boolean isNegativeCacheEnabled() { @@ -201,9 +208,12 @@ public class Groups { */ public List<String> getGroups(final String user) throws IOException { // No need to lookup for groups of static users - List<String> staticMapping = staticUserToGroupsMap.get(user); - if (staticMapping != null) { - return staticMapping; + Map<String, List<String>> staticUserToGroupsMap = staticMapRef.get(); + if (staticUserToGroupsMap != null) { + List<String> staticMapping = staticUserToGroupsMap.get(user); + if (staticMapping != null) { + return staticMapping; + } } // Check the negative cache first @@ -322,7 +332,9 @@ public class Groups { throw noGroupsForUser(user); } - return groups; + // return immutable de-duped list + return Collections.unmodifiableList( + new ArrayList<>(new LinkedHashSet<>(groups))); } /** http://git-wip-us.apache.org/repos/asf/hadoop/blob/94225152/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java ---------------------------------------------------------------------- diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java index 1e5ad36..24c75be 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java @@ -40,7 +40,6 @@ import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; -import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -1511,11 +1510,11 @@ public class UserGroupInformation { } public String getPrimaryGroupName() throws IOException { - String[] groups = getGroupNames(); - if (groups.length == 0) { + List<String> groups = getGroups(); + if (groups.isEmpty()) { throw new IOException("There is no primary group for UGI " + this); } - return groups[0]; + return groups.get(0); } /** @@ -1628,26 +1627,35 @@ public class UserGroupInformation { } /** + * Get the group names for this user. {@ #getGroups(String)} is less + * expensive alternative when checking for a contained element. + * @return the list of users with the primary group first. If the command + * fails, it returns an empty list. + */ + public String[] getGroupNames() { + List<String> groups = getGroups(); + return groups.toArray(new String[groups.size()]); + } + + /** * Get the group names for this user. * @return the list of users with the primary group first. If the command * fails, it returns an empty list. */ - public synchronized String[] getGroupNames() { + public List<String> getGroups() { ensureInitialized(); try { - Set<String> result = new LinkedHashSet<String> - (groups.getGroups(getShortUserName())); - return result.toArray(new String[result.size()]); + return groups.getGroups(getShortUserName()); } catch (IOException ie) { if (LOG.isDebugEnabled()) { LOG.debug("Failed to get groups for user " + getShortUserName() + " by " + ie); LOG.trace("TRACE", ie); } - return StringUtils.emptyStringArray; + return Collections.emptyList(); } } - + /** * Return the username. */ http://git-wip-us.apache.org/repos/asf/hadoop/blob/94225152/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/authorize/AccessControlList.java ---------------------------------------------------------------------- diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/authorize/AccessControlList.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/authorize/AccessControlList.java index b1b474b..a32a5fe 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/authorize/AccessControlList.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/authorize/AccessControlList.java @@ -231,7 +231,7 @@ public class AccessControlList implements Writable { if (allAllowed || users.contains(ugi.getShortUserName())) { return true; } else if (!groups.isEmpty()) { - for(String group: ugi.getGroupNames()) { + for (String group : ugi.getGroups()) { if (groups.contains(group)) { return true; } http://git-wip-us.apache.org/repos/asf/hadoop/blob/94225152/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java ---------------------------------------------------------------------- diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java index 8c41180..ef5c6a2 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java @@ -444,8 +444,10 @@ public class TestUserGroupInformation { UserGroupInformation uugi = UserGroupInformation.createUserForTesting(USER_NAME, GROUP_NAMES); assertEquals(USER_NAME, uugi.getUserName()); - assertArrayEquals(new String[]{GROUP1_NAME, GROUP2_NAME, GROUP3_NAME}, - uugi.getGroupNames()); + String[] expected = new String[]{GROUP1_NAME, GROUP2_NAME, GROUP3_NAME}; + assertArrayEquals(expected, uugi.getGroupNames()); + assertArrayEquals(expected, uugi.getGroups().toArray(new String[0])); + assertEquals(GROUP1_NAME, uugi.getPrimaryGroupName()); } @SuppressWarnings("unchecked") // from Mockito mocks --------------------------------------------------------------------- To unsubscribe, e-mail: common-commits-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-commits-h...@hadoop.apache.org
