Return-Path: X-Original-To: archive-asf-public-internal@cust-asf2.ponee.io Delivered-To: archive-asf-public-internal@cust-asf2.ponee.io Received: from cust-asf.ponee.io (cust-asf.ponee.io [163.172.22.183]) by cust-asf2.ponee.io (Postfix) with ESMTP id 61433200B5A for ; Thu, 4 Aug 2016 17:48:34 +0200 (CEST) Received: by cust-asf.ponee.io (Postfix) id 5FB7B160AAB; Thu, 4 Aug 2016 15:48:34 +0000 (UTC) Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by cust-asf.ponee.io (Postfix) with SMTP id 359D1160A6A for ; Thu, 4 Aug 2016 17:48:33 +0200 (CEST) Received: (qmail 73473 invoked by uid 500); 4 Aug 2016 15:48:32 -0000 Mailing-List: contact common-commits-help@hadoop.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Delivered-To: mailing list common-commits@hadoop.apache.org Received: (qmail 73464 invoked by uid 99); 4 Aug 2016 15:48:32 -0000 Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org) (140.211.11.23) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 04 Aug 2016 15:48:32 +0000 Received: by git1-us-west.apache.org (ASF Mail Server at git1-us-west.apache.org, from userid 33) id 33ECFE0A7D; Thu, 4 Aug 2016 15:48:32 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: kihwal@apache.org To: common-commits@hadoop.apache.org Message-Id: X-Mailer: ASF-Git Admin Mailer Subject: hadoop git commit: HADOOP-13442. Optimize UGI group lookups. Contributed by Daryn Sharp. Date: Thu, 4 Aug 2016 15:48:32 +0000 (UTC) archived-at: Thu, 04 Aug 2016 15:48:34 -0000 Repository: hadoop Updated Branches: refs/heads/branch-2 77b61d1f4 -> c7fe1f5ec HADOOP-13442. Optimize UGI group lookups. Contributed by Daryn Sharp. (cherry picked from commit 94225152399e6e89fa7b4cff6d17d33e544329a3) Project: http://git-wip-us.apache.org/repos/asf/hadoop/repo Commit: http://git-wip-us.apache.org/repos/asf/hadoop/commit/c7fe1f5e Tree: http://git-wip-us.apache.org/repos/asf/hadoop/tree/c7fe1f5e Diff: http://git-wip-us.apache.org/repos/asf/hadoop/diff/c7fe1f5e Branch: refs/heads/branch-2 Commit: c7fe1f5ec5bc12d8d65d509f1047b937fb44f517 Parents: 77b61d1 Author: Kihwal Lee Authored: Thu Aug 4 10:48:17 2016 -0500 Committer: Kihwal Lee Committed: Thu Aug 4 10:48:17 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/c7fe1f5e/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 88fe3eb..d79419b 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; @@ -2222,12 +2221,11 @@ public abstract class FileSystem extends Configured implements Closeable { FsPermission perm = stat.getPermission(); UserGroupInformation ugi = UserGroupInformation.getCurrentUser(); String user = ugi.getShortUserName(); - List 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/c7fe1f5e/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 5f42e70..37893d7 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 @@ -862,7 +862,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)); @@ -883,7 +883,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)); @@ -1015,7 +1015,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/c7fe1f5e/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 inode = theInternalDir.children.get(f.toUri().toString().substring(1)); @@ -863,13 +863,13 @@ public class ViewFs extends AbstractFileSystem { INodeLink inodelink = (INodeLink) 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/c7fe1f5e/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 1b092ec..ad238e6 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 @@ -23,7 +23,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; @@ -276,7 +275,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/c7fe1f5e/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 5414ba5..df32bb3 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> cache; - private final Map> staticUserToGroupsMap = - new HashMap>(); + private final AtomicReference>> 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 mappings = StringUtils.getStringCollection( staticMapping, ";"); + Map> staticUserToGroupsMap = + new HashMap>(); for (String users : mappings) { Collection 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 getGroups(final String user) throws IOException { // No need to lookup for groups of static users - List staticMapping = staticUserToGroupsMap.get(user); - if (staticMapping != null) { - return staticMapping; + Map> staticUserToGroupsMap = staticMapRef.get(); + if (staticUserToGroupsMap != null) { + List 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/c7fe1f5e/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 370f92f..e12c9a3 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 @@ -38,7 +38,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; @@ -1499,11 +1498,11 @@ public class UserGroupInformation { } public String getPrimaryGroupName() throws IOException { - String[] groups = getGroupNames(); - if (groups.length == 0) { + List groups = getGroups(); + if (groups.isEmpty()) { throw new IOException("There is no primary group for UGI " + this); } - return groups[0]; + return groups.get(0); } /** @@ -1616,26 +1615,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 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 getGroups() { ensureInitialized(); try { - Set result = new LinkedHashSet - (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/c7fe1f5e/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/c7fe1f5e/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 838d431..189a5a8 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 @@ -443,8 +443,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-unsubscribe@hadoop.apache.org For additional commands, e-mail: common-commits-help@hadoop.apache.org