HADOOP-17485. port UGI#getGroupsSet optimizations into 2.10

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java

@@ -2573,7 +2573,7 @@ static void checkAccessPermissions(FileStatus stat, FsAction mode)
       if (perm.getUserAction().implies(mode)) {
         return;
       }
-    } else if (ugi.getGroups().contains(stat.getGroup())) {
+    } else if (ugi.getGroupsSet().contains(stat.getGroup())) {
       if (perm.getGroupAction().implies(mode)) {
         return;
       }

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/SecureIOUtils.java

@@ -275,7 +275,7 @@ private static void checkStat(File f, String owner, String group,
             UserGroupInformation.createRemoteUser(expectedOwner);
         final String adminsGroupString = "Administrators";
         success = owner.equals(adminsGroupString)
-            && ugi.getGroups().contains(adminsGroupString);
+            && ugi.getGroupsSet().contains(adminsGroupString);
       } else {
         success = false;
       }

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/CompositeGroupsMapping.java

@@ -19,6 +19,7 @@
 
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
@@ -107,6 +108,29 @@ public void cacheGroupsAdd(List<String> groups) throws IOException {
     // does nothing in this provider of user to groups mapping
   }
 
+  @Override
+  public synchronized Set<String> getGroupsSet(String user) throws IOException {
+    Set<String> groupSet = new HashSet<String>();
+
+    Set<String> groups = null;
+    for (GroupMappingServiceProvider provider : providersList) {
+      try {
+        groups = provider.getGroupsSet(user);
+      } catch (Exception e) {
+        LOG.warn("Unable to get groups for user {} via {} because: {}",
+            user, provider.getClass().getSimpleName(), e.toString());
+        LOG.debug("Stacktrace: ", e);
+      }
+      if (groups != null && !groups.isEmpty()) {
+        groupSet.addAll(groups);
+        if (!combined) {
+          break;
+        }
+      }
+    }
+    return groupSet;
+  }
+
   @Override
   public synchronized Configuration getConf() {
     return conf;

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/GroupMappingServiceProvider.java

@@ -19,6 +19,7 @@
 
 import java.io.IOException;
 import java.util.List;
+import java.util.Set;
 
 import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.classification.InterfaceStability;
@@ -52,4 +53,13 @@ public interface GroupMappingServiceProvider {
    * @throws IOException
    */
   public void cacheGroupsAdd(List<String> groups) throws IOException;
+
+  /**
+   * Get all various group memberships of a given user.
+   * Returns EMPTY set in case of non-existing user
+   * @param user User's name
+   * @return set of group memberships of user
+   * @throws IOException
+   */
+  Set<String> getGroupsSet(String user) throws IOException;
 }

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Groups.java

@@ -78,8 +78,8 @@ public class Groups {
 
   private final GroupMappingServiceProvider impl;
 
-  private final LoadingCache<String, List<String>> cache;
-  private final AtomicReference<Map<String, List<String>>> staticMapRef =
+  private final LoadingCache<String, Set<String>> cache;
+  private final AtomicReference<Map<String, Set<String>>> staticMapRef =
       new AtomicReference<>();
   private final long cacheTimeout;
   private final long negativeCacheTimeout;
@@ -168,8 +168,7 @@ private void parseStaticMapping(Configuration conf) {
         CommonConfigurationKeys.HADOOP_USER_GROUP_STATIC_OVERRIDES_DEFAULT);
     Collection<String> mappings = StringUtils.getStringCollection(
         staticMapping, ";");
-    Map<String, List<String>> staticUserToGroupsMap =
-        new HashMap<String, List<String>>();
+    Map<String, Set<String>> staticUserToGroupsMap = new HashMap<>();
     for (String users : mappings) {
       Collection<String> userToGroups = StringUtils.getStringCollection(users,
           "=");
@@ -181,10 +180,10 @@ private void parseStaticMapping(Configuration conf) {
       String[] userToGroupsArray = userToGroups.toArray(new String[userToGroups
           .size()]);
       String user = userToGroupsArray[0];
-      List<String> groups = Collections.emptyList();
+      Set<String> groups = Collections.emptySet();
       if (userToGroupsArray.length == 2) {
-        groups = (List<String>) StringUtils
-            .getStringCollection(userToGroupsArray[1]);
+        groups = new LinkedHashSet(StringUtils
+            .getStringCollection(userToGroupsArray[1]));
       }
       staticUserToGroupsMap.put(user, groups);
     }
@@ -203,15 +202,47 @@ private IOException noGroupsForUser(String user) {
   /**
    * Get the group memberships of a given user.
    * If the user's group is not cached, this method may block.
+   * Note this method can be expensive as it involves Set->List conversion.
+   * For user with large group membership (i.e., > 1000 groups), we recommend
+   * using getGroupSet to avoid the conversion and fast membership look up via
+   * contains().
    * @param user User's name
-   * @return the group memberships of the user
+   * @return the group memberships of the user as list
    * @throws IOException if user does not exist
+   * @deprecated Use {@link #getGroupsSet(String user)} instead.
    */
+  @Deprecated
   public List<String> getGroups(final String user) throws IOException {
+    return Collections.unmodifiableList(new ArrayList<>(
+        getGroupInternal(user)));
+  }
+
+  /**
+   * Get the group memberships of a given user.
+   * If the user's group is not cached, this method may block.
+   * This provide better performance when user has large group membership via
+   * 1) avoid set->list->set conversion for the caller UGI/PermissionCheck
+   * 2) fast lookup using contains() via Set instead of List
+   * @param user User's name
+   * @return the group memberships of the user as set
+   * @throws IOException if user does not exist
+   */
+  public Set<String> getGroupsSet(final String user) throws IOException {
+    return Collections.unmodifiableSet(getGroupInternal(user));
+  }
+
+  /**
+   * Get the group memberships of a given user.
+   * If the user's group is not cached, this method may block.
+   * @param user User's name
+   * @return the group memberships of the user as Set
+   * @throws IOException if user does not exist
+   */
+  private Set<String> getGroupInternal(final String user) throws IOException {
     // No need to lookup for groups of static users
-    Map<String, List<String>> staticUserToGroupsMap = staticMapRef.get();
+    Map<String, Set<String>> staticUserToGroupsMap = staticMapRef.get();
     if (staticUserToGroupsMap != null) {
-      List<String> staticMapping = staticUserToGroupsMap.get(user);
+      Set<String> staticMapping = staticUserToGroupsMap.get(user);
       if (staticMapping != null) {
         return staticMapping;
       }
@@ -267,7 +298,7 @@ public long read() {
   /**
    * Deals with loading data into the cache.
    */
-  private class GroupCacheLoader extends CacheLoader<String, List<String>> {
+  private class GroupCacheLoader extends CacheLoader<String, Set<String>> {
 
     private ListeningExecutorService executorService;
 
@@ -308,17 +339,17 @@ private class GroupCacheLoader extends CacheLoader<String, List<String>> {
      * @throws IOException to prevent caching negative entries
      */
     @Override
-    public List<String> load(String user) throws Exception {
+    public Set<String> load(String user) throws Exception {
       LOG.debug("GroupCacheLoader - load.");
       TraceScope scope = null;
       Tracer tracer = Tracer.curThreadTracer();
       if (tracer != null) {
         scope = tracer.newScope("Groups#fetchGroupList");
         scope.addKVAnnotation("user", user);
       }
-      List<String> groups = null;
+      Set<String> groups = null;
       try {
-        groups = fetchGroupList(user);
+        groups = fetchGroupSet(user);
       } finally {
         if (scope != null) {
           scope.close();
@@ -334,9 +365,7 @@ public List<String> load(String user) throws Exception {
         throw noGroupsForUser(user);
       }
 
-      // return immutable de-duped list
-      return Collections.unmodifiableList(
-          new ArrayList<>(new LinkedHashSet<>(groups)));
+      return groups;
     }
 
     /**
@@ -345,28 +374,28 @@ public List<String> load(String user) throws Exception {
      * implementation, otherwise is arranges for the cache to be updated later
      */
     @Override
-    public ListenableFuture<List<String>> reload(final String key,
-                                                 List<String> oldValue)
+    public ListenableFuture<Set<String>> reload(final String key,
+                                                 Set<String> oldValue)
         throws Exception {
       LOG.debug("GroupCacheLoader - reload (async).");
       if (!reloadGroupsInBackground) {
         return super.reload(key, oldValue);
       }
 
       backgroundRefreshQueued.incrementAndGet();
-      ListenableFuture<List<String>> listenableFuture =
-          executorService.submit(new Callable<List<String>>() {
+      ListenableFuture<Set<String>> listenableFuture =
+          executorService.submit(new Callable<Set<String>>() {
             @Override
-            public List<String> call() throws Exception {
+            public Set<String> call() throws Exception {
               backgroundRefreshQueued.decrementAndGet();
               backgroundRefreshRunning.incrementAndGet();
-              List<String> results = load(key);
+              Set<String> results = GroupCacheLoader.this.load(key);
               return results;
             }
           });
-      Futures.addCallback(listenableFuture, new FutureCallback<List<String>>() {
+      Futures.addCallback(listenableFuture, new FutureCallback<Set<String>>() {
         @Override
-        public void onSuccess(List<String> result) {
+        public void onSuccess(Set<String> result) {
           backgroundRefreshSuccess.incrementAndGet();
           backgroundRefreshRunning.decrementAndGet();
         }
@@ -380,20 +409,20 @@ public void onFailure(Throwable t) {
     }
 
     /**
-     * Queries impl for groups belonging to the user. This could involve I/O and take awhile.
+     * Queries impl for groups belonging to the user.
+     * This could involve I/O and take awhile.
      */
-    private List<String> fetchGroupList(String user) throws IOException {
+    private Set<String> fetchGroupSet(String user) throws IOException {
       long startMs = timer.monotonicNow();
-      List<String> groupList = impl.getGroups(user);
+      Set<String> groups = impl.getGroupsSet(user);
       long endMs = timer.monotonicNow();
       long deltaMs = endMs - startMs ;
       UserGroupInformation.metrics.addGetGroups(deltaMs);
       if (deltaMs > warningDeltaMs) {
         LOG.warn("Potential performance problem: getGroups(user=" + user +") " +
           "took " + deltaMs + " milliseconds.");
       }
-
-      return groupList;
+      return groups;
     }
   }
 

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/JniBasedUnixGroupsMapping.java

@@ -20,8 +20,11 @@
 
 import java.io.IOException;
 import java.util.Arrays;
+import java.util.LinkedHashSet;
 import java.util.List;
+import java.util.Set;
 
+import org.apache.commons.collections.CollectionUtils;
 import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.classification.InterfaceStability;
 
@@ -75,6 +78,18 @@ static private void logError(int groupId, String error) {
 
   @Override
   public List<String> getGroups(String user) throws IOException {
+    return Arrays.asList(getGroupsInternal(user));
+  }
+
+  @Override
+  public Set<String> getGroupsSet(String user) throws IOException {
+    String[] groups = getGroupsInternal(user);
+    Set<String> result = new LinkedHashSet(groups.length);
+    CollectionUtils.addAll(result, groups);
+    return result;
+  }
+
+  private String[] getGroupsInternal(String user) throws IOException {
     String[] groups = new String[0];
     try {
       groups = getGroupsForUser(user);
@@ -85,7 +100,7 @@ public List<String> getGroups(String user) throws IOException {
         LOG.info("Error getting groups for " + user + ": " + e.getMessage());
       }
     }
-    return Arrays.asList(groups);
+    return groups;
   }
 
   @Override

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/JniBasedUnixGroupsMappingWithFallback.java

@@ -20,6 +20,7 @@
 
 import java.io.IOException;
 import java.util.List;
+import java.util.Set;
 
 import org.apache.hadoop.util.NativeCodeLoader;
 import org.apache.hadoop.util.PerformanceAdvisory;
@@ -61,4 +62,9 @@ public void cacheGroupsAdd(List<String> groups) throws IOException {
     impl.cacheGroupsAdd(groups);
   }
 
+  @Override
+  public Set<String> getGroupsSet(String user) throws IOException {
+    return impl.getGroupsSet(user);
+  }
+
 }

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/JniBasedUnixGroupsNetgroupMappingWithFallback.java

@@ -20,6 +20,7 @@
 
 import java.io.IOException;
 import java.util.List;
+import java.util.Set;
 
 import org.apache.hadoop.util.NativeCodeLoader;
 import org.slf4j.Logger;
@@ -60,4 +61,9 @@ public void cacheGroupsAdd(List<String> groups) throws IOException {
     impl.cacheGroupsAdd(groups);
   }
 
+  @Override
+  public Set<String> getGroupsSet(String user) throws IOException {
+    return impl.getGroupsSet(user);
+  }
+
 }