HADOOP-17079. Optimize UGI#getGroups by adding UGI#getGroupsSet. (#2085)

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

@@ -2695,7 +2695,7 @@ public abstract class FileSystem extends Configured
       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

@@ -272,7 +272,7 @@ public class SecureIOUtils {
             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 @@ package org.apache.hadoop.security;
 
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
@@ -106,6 +107,29 @@ public class CompositeGroupsMapping
     // 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 @@ package org.apache.hadoop.security;
 
 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

@@ -26,7 +26,6 @@ import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
-import java.util.concurrent.Callable;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.LinkedBlockingQueue;
 import java.util.concurrent.ThreadFactory;
@@ -78,8 +77,8 @@ public class Groups {
   
   private final GroupMappingServiceProvider impl;
 
-  private final LoadingCache<String, List<String>> cache;
+  private final LoadingCache<String, Set<String>> cache;
-  private final AtomicReference<Map<String, List<String>>> staticMapRef =
+  private final AtomicReference<Map<String, Set<String>>> staticMapRef =
       new AtomicReference<>();
   private final long cacheTimeout;
   private final long negativeCacheTimeout;
@@ -168,8 +167,7 @@ public class Groups {
         CommonConfigurationKeys.HADOOP_USER_GROUP_STATIC_OVERRIDES_DEFAULT);
     Collection<String> mappings = StringUtils.getStringCollection(
         staticMapping, ";");
-    Map<String, List<String>> staticUserToGroupsMap =
+    Map<String, Set<String>> staticUserToGroupsMap = new HashMap<>();
-        new HashMap<String, List<String>>();
     for (String users : mappings) {
       Collection<String> userToGroups = StringUtils.getStringCollection(users,
           "=");
@@ -181,10 +179,10 @@ public class Groups {
       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
+        groups = new LinkedHashSet(StringUtils
-            .getStringCollection(userToGroupsArray[1]);
+            .getStringCollection(userToGroupsArray[1]));
       }
       staticUserToGroupsMap.put(user, groups);
     }
@@ -203,15 +201,47 @@ public class Groups {
   /**
    * 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 List<String> getGroups(final String user) throws IOException {
+  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 +297,7 @@ public class Groups {
   /**
    * 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,7 +338,7 @@ public class Groups {
      * @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();
@@ -316,9 +346,9 @@ public class Groups {
         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 +364,7 @@ public class Groups {
         throw noGroupsForUser(user);
       }
 
-      // return immutable de-duped list
+      return groups;
-      return Collections.unmodifiableList(
-          new ArrayList<>(new LinkedHashSet<>(groups)));
     }
 
     /**
@@ -345,8 +373,8 @@ public class Groups {
      * implementation, otherwise is arranges for the cache to be updated later
      */
     @Override
-    public ListenableFuture<List<String>> reload(final String key,
+    public ListenableFuture<Set<String>> reload(final String key,
-                                                 List<String> oldValue)
+                                                 Set<String> oldValue)
         throws Exception {
       LOG.debug("GroupCacheLoader - reload (async).");
       if (!reloadGroupsInBackground) {
@@ -354,19 +382,16 @@ public class Groups {
       }
 
       backgroundRefreshQueued.incrementAndGet();
-      ListenableFuture<List<String>> listenableFuture =
+      ListenableFuture<Set<String>> listenableFuture =
-          executorService.submit(new Callable<List<String>>() {
+          executorService.submit(() -> {
-            @Override
+            backgroundRefreshQueued.decrementAndGet();
-            public List<String> call() throws Exception {
+            backgroundRefreshRunning.incrementAndGet();
-              backgroundRefreshQueued.decrementAndGet();
+            Set<String> results = load(key);
-              backgroundRefreshRunning.incrementAndGet();
+            return results;
-              List<String> results = 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,11 +405,12 @@ public class Groups {
     }
 
     /**
-     * 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);
@@ -392,8 +418,7 @@ public class Groups {
         LOG.warn("Potential performance problem: getGroups(user=" + user +") " +
           "took " + deltaMs + " milliseconds.");
       }
-
+      return groups;
-      return groupList;
     }
   }
 

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

@@ -20,8 +20,11 @@ package org.apache.hadoop.security;
 
 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 @@ public class JniBasedUnixGroupsMapping implements GroupMappingServiceProvider {
 
   @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 class JniBasedUnixGroupsMapping implements GroupMappingServiceProvider {
         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 @@ package org.apache.hadoop.security;
 
 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 class JniBasedUnixGroupsMappingWithFallback implements
     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 @@ package org.apache.hadoop.security;
 
 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 class JniBasedUnixGroupsNetgroupMappingWithFallback implements
     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/LdapGroupsMapping.java

@@ -33,6 +33,7 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Hashtable;
 import java.util.Iterator;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.HashSet;
 import java.util.Collection;
@@ -302,12 +303,12 @@ public class LdapGroupsMapping
   }
 
   private DirContext ctx;
-  private Configuration conf;
+  private volatile Configuration conf;
 
-  private Iterator<String> ldapUrls;
+  private volatile Iterator<String> ldapUrls;
   private String currentLdapUrl;
 
-  private boolean useSsl;
+  private volatile boolean useSsl;
   private String keystore;
   private String keystorePass;
   private String truststore;
@@ -320,21 +321,21 @@ public class LdapGroupsMapping
   private Iterator<BindUserInfo> bindUsers;
   private BindUserInfo currentBindUser;
 
-  private String userbaseDN;
+  private volatile String userbaseDN;
   private String groupbaseDN;
   private String groupSearchFilter;
-  private String userSearchFilter;
+  private volatile String userSearchFilter;
-  private String memberOfAttr;
+  private volatile String memberOfAttr;
   private String groupMemberAttr;
-  private String groupNameAttr;
+  private volatile String groupNameAttr;
-  private int groupHierarchyLevels;
+  private volatile int groupHierarchyLevels;
-  private String posixUidAttr;
+  private volatile String posixUidAttr;
-  private String posixGidAttr;
+  private volatile String posixGidAttr;
   private boolean isPosix;
-  private boolean useOneQuery;
+  private volatile boolean useOneQuery;
   private int numAttempts;
-  private int numAttemptsBeforeFailover;
+  private volatile int numAttemptsBeforeFailover;
-  private String ldapCtxFactoryClassName;
+  private volatile String ldapCtxFactoryClassName;
 
   /**
    * Returns list of groups for a user.
@@ -348,38 +349,7 @@ public class LdapGroupsMapping
    */
   @Override
   public synchronized List<String> getGroups(String user) {
-    /*
+    return new ArrayList<>(getGroupsSet(user));
-     * Normal garbage collection takes care of removing Context instances when
-     * they are no longer in use. Connections used by Context instances being
-     * garbage collected will be closed automatically. So in case connection is
-     * closed and gets CommunicationException, retry some times with new new
-     * DirContext/connection.
-     */
-
-    // Tracks the number of attempts made using the same LDAP server
-    int atemptsBeforeFailover = 1;
-
-    for (int attempt = 1; attempt <= numAttempts; attempt++,
-        atemptsBeforeFailover++) {
-      try {
-        return doGetGroups(user, groupHierarchyLevels);
-      } catch (AuthenticationException e) {
-        switchBindUser(e);
-      } catch (NamingException e) {
-        LOG.warn("Failed to get groups for user {} (attempt={}/{}) using {}. " +
-            "Exception: ", user, attempt, numAttempts, currentLdapUrl, e);
-        LOG.trace("TRACE", e);
-
-        if (failover(atemptsBeforeFailover, numAttemptsBeforeFailover)) {
-          atemptsBeforeFailover = 0;
-        }
-      }
-
-      // Reset ctx so that new DirContext can be created with new connection
-      this.ctx = null;
-    }
-    
-    return Collections.emptyList();
   }
 
   /**
@@ -458,10 +428,10 @@ public class LdapGroupsMapping
    * @return a list of strings representing group names of the user.
    * @throws NamingException if unable to find group names
    */
-  private List<String> lookupGroup(SearchResult result, DirContext c,
+  private Set<String> lookupGroup(SearchResult result, DirContext c,
       int goUpHierarchy)
       throws NamingException {
-    List<String> groups = new ArrayList<>();
+    Set<String> groups = new LinkedHashSet<>();
     Set<String> groupDNs = new HashSet<>();
 
     NamingEnumeration<SearchResult> groupResults;
@@ -484,11 +454,7 @@ public class LdapGroupsMapping
         getGroupNames(groupResult, groups, groupDNs, goUpHierarchy > 0);
       }
       if (goUpHierarchy > 0 && !isPosix) {
-        // convert groups to a set to ensure uniqueness
+        goUpGroupHierarchy(groupDNs, goUpHierarchy, groups);
-        Set<String> groupset = new HashSet<>(groups);
-        goUpGroupHierarchy(groupDNs, goUpHierarchy, groupset);
-        // convert set back to list for compatibility
-        groups = new ArrayList<>(groupset);
       }
     }
     return groups;
@@ -507,7 +473,7 @@ public class LdapGroupsMapping
    * return an empty string array.
    * @throws NamingException if unable to get group names
    */
-  List<String> doGetGroups(String user, int goUpHierarchy)
+  Set<String> doGetGroups(String user, int goUpHierarchy)
       throws NamingException {
     DirContext c = getDirContext();
 
@@ -518,11 +484,11 @@ public class LdapGroupsMapping
     if (!results.hasMoreElements()) {
       LOG.debug("doGetGroups({}) returned no groups because the " +
           "user is not found.", user);
-      return Collections.emptyList();
+      return Collections.emptySet();
     }
     SearchResult result = results.nextElement();
 
-    List<String> groups = Collections.emptyList();
+    Set<String> groups = Collections.emptySet();
     if (useOneQuery) {
       try {
         /**
@@ -536,7 +502,7 @@ public class LdapGroupsMapping
               memberOfAttr + "' attribute." +
               "Returned user object: " + result.toString());
         }
-        groups = new ArrayList<>();
+        groups = new LinkedHashSet<>();
         NamingEnumeration groupEnumeration = groupDNAttr.getAll();
         while (groupEnumeration.hasMore()) {
           String groupDN = groupEnumeration.next().toString();
@@ -700,6 +666,42 @@ public class LdapGroupsMapping
     // does nothing in this provider of user to groups mapping
   }
 
+  @Override
+  public Set<String> getGroupsSet(String user) {
+    /*
+     * Normal garbage collection takes care of removing Context instances when
+     * they are no longer in use. Connections used by Context instances being
+     * garbage collected will be closed automatically. So in case connection is
+     * closed and gets CommunicationException, retry some times with new new
+     * DirContext/connection.
+     */
+
+    // Tracks the number of attempts made using the same LDAP server
+    int atemptsBeforeFailover = 1;
+
+    for (int attempt = 1; attempt <= numAttempts; attempt++,
+        atemptsBeforeFailover++) {
+      try {
+        return doGetGroups(user, groupHierarchyLevels);
+      } catch (AuthenticationException e) {
+        switchBindUser(e);
+      } catch (NamingException e) {
+        LOG.warn("Failed to get groups for user {} (attempt={}/{}) using {}. " +
+            "Exception: ", user, attempt, numAttempts, currentLdapUrl, e);
+        LOG.trace("TRACE", e);
+
+        if (failover(atemptsBeforeFailover, numAttemptsBeforeFailover)) {
+          atemptsBeforeFailover = 0;
+        }
+      }
+
+      // Reset ctx so that new DirContext can be created with new connection
+      this.ctx = null;
+    }
+
+    return Collections.emptySet();
+  }
+
   @Override
   public synchronized Configuration getConf() {
     return conf;

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

@@ -15,8 +15,10 @@
  */
 package org.apache.hadoop.security;
 
+import java.io.IOException;
 import java.util.Collections;
 import java.util.List;
+import java.util.Set;
 
 /**
  * This class provides groups mapping for {@link UserGroupInformation} when the
@@ -31,6 +33,19 @@ public class NullGroupsMapping implements GroupMappingServiceProvider {
   public void cacheGroupsAdd(List<String> groups) {
   }
 
+  /**
+   * 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
+   */
+  @Override
+  public Set<String> getGroupsSet(String user) throws IOException {
+    return Collections.emptySet();
+  }
+
   /**
    * Returns an empty list.
    * @param user ignored

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

@@ -17,7 +17,6 @@
  */
 package org.apache.hadoop.security;
 
-import com.google.common.annotations.VisibleForTesting;
 import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.classification.InterfaceStability;
 import org.apache.hadoop.conf.Configuration;
@@ -25,7 +24,9 @@ import org.apache.hadoop.util.StringUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import java.util.LinkedHashSet;
 import java.util.List;
+import java.util.Set;
 import java.util.stream.Collectors;
 
 /**
@@ -88,4 +89,18 @@ public class RuleBasedLdapGroupsMapping extends LdapGroupsMapping {
     }
   }
 
+  public synchronized Set<String> getGroupsSet(String user) {
+    Set<String> groups = super.getGroupsSet(user);
+    switch (rule) {
+    case TO_UPPER:
+      return groups.stream().map(StringUtils::toUpperCase).collect(
+          Collectors.toCollection(LinkedHashSet::new));
+    case TO_LOWER:
+      return groups.stream().map(StringUtils::toLowerCase).collect(
+          Collectors.toCollection(LinkedHashSet::new));
+    case NONE:
+    default:
+      return groups;
+    }
+  }
 }

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

@@ -18,8 +18,11 @@
 package org.apache.hadoop.security;
 
 import java.io.IOException;
-import java.util.LinkedList;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.LinkedHashSet;
 import java.util.List;
+import java.util.Set;
 import java.util.StringTokenizer;
 import java.util.concurrent.TimeUnit;
 
@@ -53,7 +56,7 @@ public class ShellBasedUnixGroupsMapping extends Configured
 
   private long timeout = CommonConfigurationKeys.
       HADOOP_SECURITY_GROUP_SHELL_COMMAND_TIMEOUT_DEFAULT;
-  private static final List<String> EMPTY_GROUPS = new LinkedList<>();
+  private static final Set<String> EMPTY_GROUPS_SET = Collections.emptySet();
 
   @Override
   public void setConf(Configuration conf) {
@@ -94,7 +97,7 @@ public class ShellBasedUnixGroupsMapping extends Configured
    */
   @Override
   public List<String> getGroups(String userName) throws IOException {
-    return getUnixGroups(userName);
+    return new ArrayList(getUnixGroups(userName));
   }
 
   /**
@@ -115,6 +118,11 @@ public class ShellBasedUnixGroupsMapping extends Configured
     // does nothing in this provider of user to groups mapping
   }
 
+  @Override
+  public Set<String> getGroupsSet(String userName) throws IOException {
+    return getUnixGroups(userName);
+  }
+
   /**
    * Create a ShellCommandExecutor object using the user's name.
    *
@@ -192,44 +200,33 @@ public class ShellBasedUnixGroupsMapping extends Configured
    *         group is returned first.
    * @throws IOException if encounter any error when running the command
    */
-  private List<String> getUnixGroups(String user) throws IOException {
+  private Set<String> getUnixGroups(String user) throws IOException {
     ShellCommandExecutor executor = createGroupExecutor(user);
 
-    List<String> groups;
+    Set<String> groups;
     try {
       executor.execute();
       groups = resolveFullGroupNames(executor.getOutput());
     } catch (ExitCodeException e) {
       if (handleExecutorTimeout(executor, user)) {
-        return EMPTY_GROUPS;
+        return EMPTY_GROUPS_SET;
       } else {
         try {
           groups = resolvePartialGroupNames(user, e.getMessage(),
               executor.getOutput());
         } catch (PartialGroupNameException pge) {
           LOG.warn("unable to return groups for user {}", user, pge);
-          return EMPTY_GROUPS;
+          return EMPTY_GROUPS_SET;
         }
       }
     } catch (IOException ioe) {
       if (handleExecutorTimeout(executor, user)) {
-        return EMPTY_GROUPS;
+        return EMPTY_GROUPS_SET;
       } else {
         // If its not an executor timeout, we should let the caller handle it
         throw ioe;
       }
     }
-
-    // remove duplicated primary group
-    if (!Shell.WINDOWS) {
-      for (int i = 1; i < groups.size(); i++) {
-        if (groups.get(i).equals(groups.get(0))) {
-          groups.remove(i);
-          break;
-        }
-      }
-    }
-
     return groups;
   }
 
@@ -242,13 +239,13 @@ public class ShellBasedUnixGroupsMapping extends Configured
    * @return a linked list of group names
    * @throws PartialGroupNameException
    */
-  private List<String> parsePartialGroupNames(String groupNames,
+  private Set<String> parsePartialGroupNames(String groupNames,
       String groupIDs) throws PartialGroupNameException {
     StringTokenizer nameTokenizer =
         new StringTokenizer(groupNames, Shell.TOKEN_SEPARATOR_REGEX);
     StringTokenizer idTokenizer =
         new StringTokenizer(groupIDs, Shell.TOKEN_SEPARATOR_REGEX);
-    List<String> groups = new LinkedList<String>();
+    Set<String> groups = new LinkedHashSet<>();
     while (nameTokenizer.hasMoreTokens()) {
       // check for unresolvable group names.
       if (!idTokenizer.hasMoreTokens()) {
@@ -277,10 +274,10 @@ public class ShellBasedUnixGroupsMapping extends Configured
    * @param userName the user's name
    * @param errMessage error message from the shell command
    * @param groupNames the incomplete list of group names
-   * @return a list of resolved group names
+   * @return a set of resolved group names
    * @throws PartialGroupNameException if the resolution fails or times out
    */
-  private List<String> resolvePartialGroupNames(String userName,
+  private Set<String> resolvePartialGroupNames(String userName,
       String errMessage, String groupNames) throws PartialGroupNameException {
     // Exception may indicate that some group names are not resolvable.
     // Shell-based implementation should tolerate unresolvable groups names,
@@ -322,16 +319,16 @@ public class ShellBasedUnixGroupsMapping extends Configured
   }
 
   /**
-   * Split group names into a linked list.
+   * Split group names into a set.
    *
    * @param groupNames a string representing the user's group names
-   * @return a linked list of group names
+   * @return a set of group names
    */
   @VisibleForTesting
-  protected List<String> resolveFullGroupNames(String groupNames) {
+  protected Set<String> resolveFullGroupNames(String groupNames) {
     StringTokenizer tokenizer =
         new StringTokenizer(groupNames, Shell.TOKEN_SEPARATOR_REGEX);
-    List<String> groups = new LinkedList<String>();
+    Set<String> groups = new LinkedHashSet<>();
     while (tokenizer.hasMoreTokens()) {
       groups.add(tokenizer.nextToken());
     }

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

@@ -40,7 +40,6 @@ import java.security.PrivilegedAction;
 import java.security.PrivilegedActionException;
 import java.security.PrivilegedExceptionAction;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.EnumMap;
@@ -1483,8 +1482,8 @@ public class UserGroupInformation {
    * map that has the translation of usernames to groups.
    */
   private static class TestingGroups extends Groups {
-    private final Map<String, List<String>> userToGroupsMapping = 
+    private final Map<String, Set<String>> userToGroupsMapping =
-      new HashMap<String,List<String>>();
+        new HashMap<>();
     private Groups underlyingImplementation;
     
     private TestingGroups(Groups underlyingImplementation) {
@@ -1494,17 +1493,22 @@ public class UserGroupInformation {
     
     @Override
     public List<String> getGroups(String user) throws IOException {
-      List<String> result = userToGroupsMapping.get(user);
+      return new ArrayList<>(getGroupsSet(user));
-      
+    }
-      if (result == null) {
-        result = underlyingImplementation.getGroups(user);
-      }
 
+    @Override
+    public Set<String> getGroupsSet(String user) throws IOException {
+      Set<String> result = userToGroupsMapping.get(user);
+      if (result == null) {
+        result = underlyingImplementation.getGroupsSet(user);
+      }
       return result;
     }
 
     private void setUserGroups(String user, String[] groups) {
-      userToGroupsMapping.put(user, Arrays.asList(groups));
+      Set<String> groupsSet = new LinkedHashSet<>();
+      Collections.addAll(groupsSet, groups);
+      userToGroupsMapping.put(user, groupsSet);
     }
   }
 
@@ -1563,11 +1567,11 @@ public class UserGroupInformation {
   }
 
   public String getPrimaryGroupName() throws IOException {
-    List<String> groups = getGroups();
+    Set<String> groupsSet = getGroupsSet();
-    if (groups.isEmpty()) {
+    if (groupsSet.isEmpty()) {
       throw new IOException("There is no primary group for UGI " + this);
     }
-    return groups.get(0);
+    return groupsSet.iterator().next();
   }
 
   /**
@@ -1680,21 +1684,24 @@ public class UserGroupInformation {
   }
 
   /**
-   * Get the group names for this user. {@link #getGroups()} is less
+   * Get the group names for this user. {@link #getGroupsSet()} 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();
+    Collection<String> groupsSet = getGroupsSet();
-    return groups.toArray(new String[groups.size()]);
+    return groupsSet.toArray(new String[groupsSet.size()]);
   }
 
   /**
-   * Get the group names for this user.
+   * Get the group names for this user. {@link #getGroupsSet()} 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.
+   * @deprecated Use {@link #getGroupsSet()} instead.
    */
+  @Deprecated
   public List<String> getGroups() {
     ensureInitialized();
     try {
@@ -1705,6 +1712,21 @@ public class UserGroupInformation {
     }
   }
 
+  /**
+   * Get the groups names for the user as a Set.
+   * @return the set of users with the primary group first. If the command
+   *     fails, it returns an empty set.
+   */
+  public Set<String> getGroupsSet() {
+    ensureInitialized();
+    try {
+      return groups.getGroupsSet(getShortUserName());
+    } catch (IOException ie) {
+      LOG.debug("Failed to get groups for user {}", getShortUserName(), ie);
+      return Collections.emptySet();
+    }
+  }
+
   /**
    * Return the username.
    */

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

@@ -24,6 +24,7 @@ import java.util.Collection;
 import java.util.HashSet;
 import java.util.LinkedList;
 import java.util.List;
+import java.util.Set;
 
 import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.classification.InterfaceStability;
@@ -231,8 +232,9 @@ public class AccessControlList implements Writable {
     if (allAllowed || users.contains(ugi.getShortUserName())) {
       return true;
     } else if (!groups.isEmpty()) {
-      for (String group : ugi.getGroups()) {
+      Set<String> ugiGroups = ugi.getGroupsSet();
-        if (groups.contains(group)) {
+      for (String group : groups) {
+        if (ugiGroups.contains(group)) {
           return true;
         }
       }

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/http/TestHttpServer.java

@@ -62,8 +62,10 @@ import java.net.URL;
 import java.util.Arrays;
 import java.util.Enumeration;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.SortedSet;
 import java.util.TreeSet;
 import java.util.concurrent.CountDownLatch;
@@ -410,6 +412,13 @@ public class TestHttpServer extends HttpServerFunctionalTest {
     public List<String> getGroups(String user) throws IOException {
       return mapping.get(user);
     }
+
+    @Override
+    public Set<String> getGroupsSet(String user) throws IOException {
+      Set<String> result = new HashSet();
+      result.addAll(mapping.get(user));
+      return result;
+    }
   }
 
   /**

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCompositeGroupMapping.java

@@ -22,7 +22,9 @@ import static org.junit.Assert.assertTrue;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 
 import org.apache.hadoop.conf.Configurable;
 import org.apache.hadoop.conf.Configuration;
@@ -87,13 +89,22 @@ public class TestCompositeGroupMapping {
     public void cacheGroupsAdd(List<String> groups) throws IOException {
       
     }
-    
+
     protected List<String> toList(String group) {
       if (group != null) {
         return Arrays.asList(new String[] {group});
       }
       return new ArrayList<String>();
     }
+
+    protected Set<String> toSet(String group) {
+      if (group != null) {
+        Set<String> result = new HashSet<>();
+        result.add(group);
+        return result;
+      }
+      return new HashSet<String>();
+    }
     
     protected void checkTestConf(String expectedValue) {
       String configValue = getConf().get(PROVIDER_SPECIFIC_CONF_KEY);
@@ -106,32 +117,49 @@ public class TestCompositeGroupMapping {
   private static class UserProvider extends GroupMappingProviderBase {
     @Override
     public List<String> getGroups(String user) throws IOException {
+      return toList(getGroupInternal(user));
+    }
+
+    @Override
+    public Set<String> getGroupsSet(String user) throws IOException {
+      return toSet(getGroupInternal(user));
+    }
+
+    private String getGroupInternal(String user) throws IOException {
       checkTestConf(PROVIDER_SPECIFIC_CONF_VALUE_FOR_USER);
-      
+
       String group = null;
       if (user.equals(john.name)) {
         group = john.group;
       } else if (user.equals(jack.name)) {
         group = jack.group;
       }
-      
+      return group;
-      return toList(group);
     }
   }
   
   private static class ClusterProvider extends GroupMappingProviderBase {    
     @Override
     public List<String> getGroups(String user) throws IOException {
+      return toList(getGroupsInternal(user));
+    }
+
+    @Override
+    public Set<String> getGroupsSet(String user) throws IOException {
+      return toSet(getGroupsInternal(user));
+    }
+
+    private String getGroupsInternal(String user) throws IOException {
       checkTestConf(PROVIDER_SPECIFIC_CONF_VALUE_FOR_CLUSTER);
-      
+
       String group = null;
       if (user.equals(hdfs.name)) {
         group = hdfs.group;
       } else if (user.equals(jack.name)) { // jack has another group from clusterProvider
         group = jack.group2;
       }
-      
+      return group;
-      return toList(group);
+
     }
   }
   

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestGroupsCaching.java

@@ -21,9 +21,9 @@ import java.io.IOException;
 
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashSet;
 import java.util.LinkedHashSet;
-import java.util.LinkedList;
 import java.util.List;
 import java.util.Set;
 import java.util.concurrent.CountDownLatch;
@@ -75,7 +75,7 @@ public class TestGroupsCaching {
     private static volatile CountDownLatch latch = null;
 
     @Override
-    public List<String> getGroups(String user) throws IOException {
+    public Set<String> getGroupsSet(String user) throws IOException {
       TESTLOG.info("Getting groups for " + user);
       delayIfNecessary();
 
@@ -86,9 +86,14 @@ public class TestGroupsCaching {
       }
 
       if (blackList.contains(user)) {
-        return new LinkedList<String>();
+        return Collections.emptySet();
       }
-      return new LinkedList<String>(allGroups);
+      return new LinkedHashSet<>(allGroups);
+    }
+
+    @Override
+    public List<String> getGroups(String user) throws IOException {
+      return new ArrayList<>(getGroupsSet(user));
     }
 
     /**
@@ -129,7 +134,7 @@ public class TestGroupsCaching {
       TESTLOG.info("Resetting FakeGroupMapping");
       blackList.clear();
       allGroups.clear();
-      requestCount = 0;
+      resetRequestCount();
       getGroupsDelayMs = 0;
       throwException = false;
       latch = null;
@@ -197,6 +202,12 @@ public class TestGroupsCaching {
       throw new IOException("For test");
     }
 
+    @Override
+    public Set<String> getGroupsSet(String user) throws IOException {
+      requestCount++;
+      throw new IOException("For test");
+    }
+
     public static int getRequestCount() {
       return requestCount;
     }
@@ -550,7 +561,7 @@ public class TestGroupsCaching {
     FakeGroupMapping.clearBlackList();
 
     // We make an initial request to populate the cache
-    groups.getGroups("me");
+    List<String> g1 = groups.getGroups("me");
 
     // add another group
     groups.cacheGroupsAdd(Arrays.asList("grp3"));

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestRuleBasedLdapGroupsMapping.java

@@ -24,7 +24,9 @@ import org.mockito.Mockito;
 
 import javax.naming.NamingException;
 import java.util.ArrayList;
+import java.util.LinkedHashSet;
 import java.util.List;
+import java.util.Set;
 
 import static org.apache.hadoop.security.RuleBasedLdapGroupsMapping
     .CONVERSION_RULE_KEY;
@@ -40,7 +42,7 @@ public class TestRuleBasedLdapGroupsMapping  {
   public void testGetGroupsToUpper() throws NamingException {
     RuleBasedLdapGroupsMapping groupsMapping = Mockito.spy(
         new RuleBasedLdapGroupsMapping());
-    List<String> groups = new ArrayList<>();
+    Set<String> groups = new LinkedHashSet<>();
     groups.add("group1");
     groups.add("group2");
     Mockito.doReturn(groups).when((LdapGroupsMapping) groupsMapping)
@@ -61,7 +63,7 @@ public class TestRuleBasedLdapGroupsMapping  {
   public void testGetGroupsToLower() throws NamingException {
     RuleBasedLdapGroupsMapping groupsMapping = Mockito.spy(
         new RuleBasedLdapGroupsMapping());
-    List<String> groups = new ArrayList<>();
+    Set<String> groups = new LinkedHashSet<>();
     groups.add("GROUP1");
     groups.add("GROUP2");
     Mockito.doReturn(groups).when((LdapGroupsMapping) groupsMapping)
@@ -82,7 +84,7 @@ public class TestRuleBasedLdapGroupsMapping  {
   public void testGetGroupsInvalidRule() throws NamingException {
     RuleBasedLdapGroupsMapping groupsMapping = Mockito.spy(
         new RuleBasedLdapGroupsMapping());
-    List<String> groups = new ArrayList<>();
+    Set<String> groups = new LinkedHashSet<>();
     groups.add("group1");
     groups.add("GROUP2");
     Mockito.doReturn(groups).when((LdapGroupsMapping) groupsMapping)
@@ -93,7 +95,7 @@ public class TestRuleBasedLdapGroupsMapping  {
     conf.set(CONVERSION_RULE_KEY, "none");
     groupsMapping.setConf(conf);
 
-    Assert.assertEquals(groups, groupsMapping.getGroups("admin"));
+    Assert.assertEquals(groups, groupsMapping.getGroupsSet("admin"));
   }
 
 }

hadoop-hdfs-project/hadoop-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/HttpFSServer.java

@@ -96,6 +96,7 @@ import java.text.MessageFormat;
 import java.util.EnumSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 /**
  * Main class of HttpFSServer server.
@@ -288,7 +289,7 @@ public class HttpFSServer {
     case INSTRUMENTATION: {
       enforceRootPath(op.value(), path);
       Groups groups = HttpFSServerWebApp.get().get(Groups.class);
-      List<String> userGroups = groups.getGroups(user.getShortUserName());
+      Set<String> userGroups = groups.getGroupsSet(user.getShortUserName());
       if (!userGroups.contains(HttpFSServerWebApp.get().getAdminGroup())) {
         throw new AccessControlException(
             "User not in HttpFSServer admin group");

hadoop-hdfs-project/hadoop-hdfs-httpfs/src/main/java/org/apache/hadoop/lib/service/Groups.java

@@ -22,10 +22,13 @@ import org.apache.hadoop.classification.InterfaceAudience;
 
 import java.io.IOException;
 import java.util.List;
+import java.util.Set;
 
 @InterfaceAudience.Private
 public interface Groups {
 
   public List<String> getGroups(String user) throws IOException;
 
+  Set<String> getGroupsSet(String user) throws IOException;
+
 }

hadoop-hdfs-project/hadoop-hdfs-httpfs/src/main/java/org/apache/hadoop/lib/service/security/GroupsService.java

@@ -27,6 +27,7 @@ import org.apache.hadoop.lib.util.ConfigurationUtils;
 
 import java.io.IOException;
 import java.util.List;
+import java.util.Set;
 
 @InterfaceAudience.Private
 public class GroupsService extends BaseService implements Groups {
@@ -50,9 +51,18 @@ public class GroupsService extends BaseService implements Groups {
     return Groups.class;
   }
 
+  /**
+   * @deprecated use {@link #getGroupsSet(String user)}
+   */
+  @Deprecated
   @Override
   public List<String> getGroups(String user) throws IOException {
     return hGroups.getGroups(user);
   }
 
+  @Override
+  public Set<String> getGroupsSet(String user) throws IOException {
+    return hGroups.getGroupsSet(user);
+  }
+
 }

hadoop-hdfs-project/hadoop-hdfs-httpfs/src/test/java/org/apache/hadoop/fs/http/server/TestHttpFSServer.java

@@ -60,9 +60,11 @@ import java.nio.charset.Charset;
 import java.text.MessageFormat;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 import org.apache.commons.io.IOUtils;
 import org.apache.hadoop.conf.Configuration;
@@ -170,6 +172,11 @@ public class TestHttpFSServer extends HFSTestCase {
       return Arrays.asList(HadoopUsersConfTestHelper.getHadoopUserGroups(user));
     }
 
+    @Override
+    public Set<String> getGroupsSet(String user) throws IOException {
+      return new HashSet<>(getGroups(user));
+    }
+
   }
 
   private Configuration createHttpFSConf(boolean addDelegationTokenAuthHandler,

hadoop-hdfs-project/hadoop-hdfs-httpfs/src/test/java/org/apache/hadoop/lib/service/security/DummyGroupMapping.java

@@ -21,7 +21,9 @@ import java.io.IOException;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
+import java.util.Set;
 
+import com.google.common.collect.Sets;
 import org.apache.hadoop.security.GroupMappingServiceProvider;
 import org.apache.hadoop.test.HadoopUsersConfTestHelper;
 
@@ -47,4 +49,17 @@ public class DummyGroupMapping implements GroupMappingServiceProvider {
   @Override
   public void cacheGroupsAdd(List<String> groups) throws IOException {
   }
+
+  @Override
+  public Set<String> getGroupsSet(String user) throws IOException {
+    if (user.equals("root")) {
+      return Sets.newHashSet("admin");
+    } else if (user.equals("nobody")) {
+      return Sets.newHashSet("nobody");
+    } else {
+      String[] groups = HadoopUsersConfTestHelper.getHadoopUserGroups(user);
+      return (groups != null) ? Sets.newHashSet(groups) :
+          Collections.emptySet();
+    }
+  }
 }

hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterPermissionChecker.java

@@ -18,8 +18,6 @@
 package org.apache.hadoop.hdfs.server.federation.router;
 
 import java.io.IOException;
-import java.util.Arrays;
-import java.util.List;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -126,8 +124,7 @@ public class RouterPermissionChecker extends FSPermissionChecker {
     }
 
     // Is the user a member of the super group?
-    List<String> groups = ugi.getGroups();
+    if (ugi.getGroupsSet().contains(superGroup)) {
-    if (groups.contains(superGroup)) {
       return;
     }
 

hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/store/records/MountTable.java

@@ -149,7 +149,7 @@ public abstract class MountTable extends BaseRecord {
     // Set permission fields
     UserGroupInformation ugi = NameNode.getRemoteUser();
     record.setOwnerName(ugi.getShortUserName());
-    String group = ugi.getGroups().isEmpty() ? ugi.getShortUserName()
+    String group = ugi.getGroupsSet().isEmpty() ? ugi.getShortUserName()
         : ugi.getPrimaryGroupName();
     record.setGroupName(group);
     record.setMode(new FsPermission(

hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterRefreshSuperUserGroupsConfiguration.java

@@ -45,6 +45,7 @@ import java.net.URL;
 import java.net.URLDecoder;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.LinkedHashSet;
 
 import static org.junit.Assert.assertEquals;
 import static org.mockito.Mockito.mock;
@@ -135,6 +136,8 @@ public class TestRouterRefreshSuperUserGroupsConfiguration {
     when(ugi.getRealUser()).thenReturn(impersonator);
     when(ugi.getUserName()).thenReturn("victim");
     when(ugi.getGroups()).thenReturn(Arrays.asList("groupVictim"));
+    when(ugi.getGroupsSet()).thenReturn(new LinkedHashSet<>(Arrays.asList(
+        "groupVictim")));
 
     // Exception should be thrown before applying config
     LambdaTestUtils.intercept(

hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterUserMappings.java

@@ -18,6 +18,7 @@
 
 package org.apache.hadoop.hdfs.server.federation.router;
 
+import com.google.common.collect.Sets;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.CommonConfigurationKeys;
 import org.apache.hadoop.fs.FileSystem;
@@ -56,7 +57,9 @@ import java.io.UnsupportedEncodingException;
 import java.net.URL;
 import java.net.URLDecoder;
 import java.util.ArrayList;
+import java.util.LinkedHashSet;
 import java.util.List;
+import java.util.Set;
 import java.util.concurrent.TimeUnit;
 
 import static org.junit.Assert.assertArrayEquals;
@@ -111,6 +114,16 @@ public class TestRouterUserMappings {
     @Override
     public void cacheGroupsAdd(List<String> groups) throws IOException {
     }
+
+    @Override
+    public Set<String> getGroupsSet(String user) throws IOException {
+      LOG.info("Getting groups in MockUnixGroupsMapping");
+      String g1 = user + (10 * i + 1);
+      String g2 = user + (10 * i + 2);
+      Set<String> s = Sets.newHashSet(g1, g2);
+      i++;
+      return s;
+    }
   }
 
   @Before
@@ -191,6 +204,10 @@ public class TestRouterUserMappings {
     final List<String> groupNames2 = new ArrayList<>();
     groupNames2.add("gr3");
     groupNames2.add("gr4");
+    final Set<String> groupNamesSet1 = new LinkedHashSet<>();
+    groupNamesSet1.addAll(groupNames1);
+    final Set<String> groupNamesSet2 = new LinkedHashSet<>();
+    groupNamesSet2.addAll(groupNames2);
 
     //keys in conf
     String userKeyGroups = DefaultImpersonationProvider.getTestProvider().
@@ -222,6 +239,8 @@ public class TestRouterUserMappings {
     // set groups for users
     when(ugi1.getGroups()).thenReturn(groupNames1);
     when(ugi2.getGroups()).thenReturn(groupNames2);
+    when(ugi1.getGroupsSet()).thenReturn(groupNamesSet1);
+    when(ugi2.getGroupsSet()).thenReturn(groupNamesSet2);
 
     // check before refresh
     LambdaTestUtils.intercept(AuthorizationException.class,

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java

@@ -1082,8 +1082,7 @@ public class DataNode extends ReconfigurableBase
     }
 
     // Is the user a member of the super group?
-    List<String> groups = callerUgi.getGroups();
+    if (callerUgi.getGroupsSet().contains(supergroup)) {
-    if (groups.contains(supergroup)) {
       return;
     }
     // Not a superuser.

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSPermissionChecker.java

@@ -103,7 +103,7 @@ public class FSPermissionChecker implements AccessControlEnforcer {
     this.fsOwner = fsOwner;
     this.supergroup = supergroup;
     this.callerUgi = callerUgi;
-    this.groups = callerUgi.getGroups();
+    this.groups = callerUgi.getGroupsSet();
     user = callerUgi.getShortUserName();
     isSuper = user.equals(fsOwner) || groups.contains(supergroup);
     this.attributeProvider = attributeProvider;

hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/security/TestRefreshUserMappings.java

@@ -34,8 +34,11 @@ import java.io.UnsupportedEncodingException;
 import java.net.URL;
 import java.net.URLDecoder;
 import java.util.ArrayList;
+import java.util.LinkedHashSet;
 import java.util.List;
+import java.util.Set;
 
+import com.google.common.collect.Sets;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.FileSystem;
 import org.apache.hadoop.fs.Path;
@@ -84,6 +87,16 @@ public class TestRefreshUserMappings {
     @Override
     public void cacheGroupsAdd(List<String> groups) throws IOException {
     }
+
+    @Override
+    public Set<String> getGroupsSet(String user) {
+      LOG.info("Getting groups in MockUnixGroupsMapping");
+      String g1 = user + (10 * i + 1);
+      String g2 = user + (10 * i + 2);
+      Set<String> s = Sets.newHashSet(g1, g2);
+      i++;
+      return s;
+    }
   }
   
   @Before
@@ -196,6 +209,8 @@ public class TestRefreshUserMappings {
     // set groups for users
     when(ugi1.getGroups()).thenReturn(groupNames1);
     when(ugi2.getGroups()).thenReturn(groupNames2);
+    when(ugi1.getGroupsSet()).thenReturn(new LinkedHashSet<>(groupNames1));
+    when(ugi2.getGroupsSet()).thenReturn(new LinkedHashSet<>(groupNames2));
 
 
     // check before

hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-hs/src/test/java/org/apache/hadoop/mapreduce/v2/hs/server/TestHSAdminServer.java

@@ -26,7 +26,9 @@ import java.security.PrivilegedExceptionAction;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.LinkedHashSet;
 import java.util.List;
+import java.util.Set;
 
 import org.apache.hadoop.HadoopIllegalArgumentException;
 import org.apache.hadoop.conf.Configuration;
@@ -56,6 +58,7 @@ import static org.mockito.Mockito.verify;
 
 import org.apache.hadoop.security.authorize.AuthorizationException;
 import org.apache.hadoop.yarn.logaggregation.AggregatedLogDeletionService;
+import org.mockito.internal.util.collections.Sets;
 
 @RunWith(Parameterized.class)
 public class TestHSAdminServer {
@@ -91,6 +94,15 @@ public class TestHSAdminServer {
     @Override
     public void cacheGroupsAdd(List<String> groups) throws IOException {
     }
+
+    @Override
+    public Set<String> getGroupsSet(String user) throws IOException {
+      Set<String> result = new LinkedHashSet<>();
+      result.add(user + (10 * i + 1));
+      result.add(user + (10 * i +2));
+      i++;
+      return result;
+    }
   }
 
   @Parameters
@@ -189,6 +201,9 @@ public class TestHSAdminServer {
     when(superUser.getUserName()).thenReturn("superuser");
     when(ugi.getGroups())
         .thenReturn(Arrays.asList(new String[] { "group3" }));
+    when(ugi.getGroupsSet())
+        .thenReturn(Sets.newSet("group3"));
+
     when(ugi.getUserName()).thenReturn("regularUser");
 
     // Set super user groups not to include groups of regularUser

hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-hs/src/test/java/org/apache/hadoop/mapreduce/v2/hs/webapp/TestHsWebServicesAcls.java

@@ -28,6 +28,7 @@ import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
@@ -276,6 +277,11 @@ public class TestHsWebServicesAcls {
     @Override
     public void cacheGroupsAdd(List<String> groups) throws IOException {
     }
+
+    @Override
+    public Set<String> getGroupsSet(String user) throws IOException {
+      return Collections.emptySet();
+    }
   }
 
   private static class MockJobForAcls implements Job {

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/linux/resources/NetworkTagMappingJsonManager.java

@@ -86,7 +86,7 @@ public class NetworkTagMappingJsonManager implements NetworkTagMappingManager {
         container.getUser());
     List<Group> groups = this.networkTagMapping.getGroups();
     for(Group group : groups) {
-      if (userUGI.getGroups().contains(group.getGroupName())) {
+      if (userUGI.getGroupsSet().contains(group.getGroupName())) {
         return group.getNetworkTagID();
       }
     }

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/linux/runtime/JavaSandboxLinuxContainerRuntime.java

@@ -303,9 +303,9 @@ public class JavaSandboxLinuxContainerRuntime
   private static List<String> getGroupPolicyFiles(Configuration conf,
       String user) throws ContainerExecutionException {
     Groups groups = Groups.getUserToGroupsMappingService(conf);
-    List<String> userGroups;
+    Set<String> userGroups;
     try {
-      userGroups = groups.getGroups(user);
+      userGroups = groups.getGroupsSet(user);
     } catch (IOException e) {
       throw new ContainerExecutionException("Container user does not exist");
     }
@@ -330,11 +330,11 @@ public class JavaSandboxLinuxContainerRuntime
     String whitelistGroup = configuration.get(
         YarnConfiguration.YARN_CONTAINER_SANDBOX_WHITELIST_GROUP);
     Groups groups = Groups.getUserToGroupsMappingService(configuration);
-    List<String> userGroups;
+    Set<String> userGroups;
     boolean isWhitelisted = false;
 
     try {
-      userGroups = groups.getGroups(username);
+      userGroups = groups.getGroupsSet(username);
     } catch (IOException e) {
       throw new ContainerExecutionException("Container user does not exist");
     }

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/PrimaryGroupPlacementRule.java

@@ -30,7 +30,7 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
-import java.util.List;
+import java.util.Set;
 
 import static org.apache.hadoop.yarn.server.resourcemanager.placement.FairQueuePlacementUtils.DOT;
 import static org.apache.hadoop.yarn.server.resourcemanager.placement.FairQueuePlacementUtils.assureRoot;
@@ -62,19 +62,19 @@ public class PrimaryGroupPlacementRule extends FSPlacementRule {
 
     // All users should have at least one group the primary group. If no groups
     // are returned then there is a real issue.
-    final List<String> groupList;
+    final Set<String> groupSet;
     try {
-      groupList = groupProvider.getGroups(user);
+      groupSet = groupProvider.getGroupsSet(user);
     } catch (IOException ioe) {
       throw new YarnException("Group resolution failed", ioe);
     }
-    if (groupList.isEmpty()) {
+    if (groupSet.isEmpty()) {
       LOG.error("Group placement rule failed: No groups returned for user {}",
           user);
       throw new YarnException("No groups returned for user " + user);
     }
 
-    String cleanGroup = cleanName(groupList.get(0));
+    String cleanGroup = cleanName(groupSet.iterator().next());
     String queueName;
     PlacementRule parentRule = getParentRule();
 

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/SecondaryGroupExistingPlacementRule.java

@@ -30,7 +30,8 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
-import java.util.List;
+import java.util.Iterator;
+import java.util.Set;
 
 import static org.apache.hadoop.yarn.server.resourcemanager.placement.FairQueuePlacementUtils.DOT;
 import static org.apache.hadoop.yarn.server.resourcemanager.placement.FairQueuePlacementUtils.assureRoot;
@@ -65,9 +66,9 @@ public class SecondaryGroupExistingPlacementRule extends FSPlacementRule {
 
     // All users should have at least one group the primary group. If no groups
     // are returned then there is a real issue.
-    final List<String> groupList;
+    final Set<String> groupSet;
     try {
-      groupList = groupProvider.getGroups(user);
+      groupSet = groupProvider.getGroupsSet(user);
     } catch (IOException ioe) {
       throw new YarnException("Group resolution failed", ioe);
     }
@@ -90,8 +91,9 @@ public class SecondaryGroupExistingPlacementRule extends FSPlacementRule {
           parentQueue);
     }
     // now check the groups inside the parent
-    for (int i = 1; i < groupList.size(); i++) {
+    Iterator<String> it = groupSet.iterator();
-      String group = cleanName(groupList.get(i));
+    while (it.hasNext()) {
+      String group = cleanName(it.next());
       String queueName =
           parentQueue == null ? assureRoot(group) : parentQueue + DOT + group;
       if (configuredQueue(queueName)) {

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/UserGroupMappingPlacementRule.java

@@ -20,7 +20,9 @@ package org.apache.hadoop.yarn.server.resourcemanager.placement;
 
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.Iterator;
 import java.util.List;
+import java.util.Set;
 
 import org.apache.commons.lang3.StringUtils;
 import org.apache.hadoop.classification.InterfaceAudience.Private;
@@ -74,18 +76,21 @@ public class UserGroupMappingPlacementRule extends PlacementRule {
   }
 
   private String getPrimaryGroup(String user) throws IOException {
-    return groups.getGroups(user).get(0);
+    return groups.getGroupsSet(user).iterator().next();
   }
 
   private String getSecondaryGroup(String user) throws IOException {
-    List<String> groupsList = groups.getGroups(user);
+    Set<String> groupsSet = groups.getGroupsSet(user);
     String secondaryGroup = null;
     // Traverse all secondary groups (as there could be more than one
     // and position is not guaranteed) and ensure there is queue with
     // the same name
-    for (int i = 1; i < groupsList.size(); i++) {
+    Iterator<String> it = groupsSet.iterator();
-      if (this.queueManager.getQueue(groupsList.get(i)) != null) {
+    it.next();
-        secondaryGroup = groupsList.get(i);
+    while (it.hasNext()) {
+      String group = it.next();
+      if (this.queueManager.getQueue(group) != null) {
+        secondaryGroup = group;
         break;
       }
     }
@@ -180,7 +185,7 @@ public class UserGroupMappingPlacementRule extends PlacementRule {
         }
       }
       if (mapping.getType().equals(MappingType.GROUP)) {
-        for (String userGroups : groups.getGroups(user)) {
+        for (String userGroups : groups.getGroupsSet(user)) {
           if (userGroups.equals(mapping.getSource())) {
             if (mapping.getQueue().equals(CURRENT_USER_MAPPING)) {
               if (LOG.isDebugEnabled()) {

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestRMAdminService.java

@@ -1459,6 +1459,11 @@ public class TestRMAdminService {
       // Do nothing
     }
 
+    @Override
+    public Set<String> getGroupsSet(String user) throws IOException {
+      return ImmutableSet.copyOf(group);
+    }
+
     public static void updateGroups() {
       group.clear();
       group.add("test_group_D");

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/PeriodGroupsMapping.java

@@ -18,17 +18,20 @@
 
 package org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair;
 
+import com.google.common.collect.ImmutableSet;
 import org.apache.hadoop.security.GroupMappingServiceProvider;
 
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.List;
+import java.util.Set;
 
 public class PeriodGroupsMapping implements GroupMappingServiceProvider {
   
   @Override
   public List<String> getGroups(String user) {
-    return Arrays.asList(user + ".group", user + "subgroup1", user + "subgroup2");
+    return Arrays.asList(user + ".group", user + "subgroup1",
+        user + "subgroup2");
   }
 
   @Override
@@ -41,4 +44,9 @@ public class PeriodGroupsMapping implements GroupMappingServiceProvider {
     throw new UnsupportedOperationException();
   }
 
+  @Override
+  public Set<String> getGroupsSet(String user) throws IOException {
+    return ImmutableSet.of(user + ".group", user + "subgroup1",
+        user + "subgroup2");
+  }
 }

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/PrimaryGroupMapping.java

@@ -22,7 +22,9 @@ import org.apache.hadoop.security.GroupMappingServiceProvider;
 
 import java.io.IOException;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.List;
+import java.util.Set;
 
 /**
  * Group Mapping class used for test cases. Returns only primary group of the
@@ -44,4 +46,9 @@ public class PrimaryGroupMapping implements GroupMappingServiceProvider {
   public void cacheGroupsAdd(List<String> groups) throws IOException {
     throw new UnsupportedOperationException();
   }
+
+  @Override
+  public Set<String> getGroupsSet(String user) throws IOException {
+    return Collections.singleton(user + "group");
+  }
 }

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/SimpleGroupsMapping.java

@@ -21,7 +21,9 @@ package org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair;
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.List;
+import java.util.Set;
 
+import com.google.common.collect.ImmutableSet;
 import org.apache.hadoop.security.GroupMappingServiceProvider;
 
 public class SimpleGroupsMapping implements GroupMappingServiceProvider {
@@ -45,4 +47,10 @@ public class SimpleGroupsMapping implements GroupMappingServiceProvider {
   @Override
   public void cacheGroupsAdd(List<String> groups) throws IOException {
   }
+
+  @Override
+  public Set<String> getGroupsSet(String user) throws IOException {
+    return ImmutableSet.of(user + "group", user + "subgroup1",
+        user + "subgroup2");
+  }
 }