xiaoyuyao commented on a change in pull request #2085:
URL: https://github.com/apache/hadoop/pull/2085#discussion_r446313125
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Groups.java
##########
@@ -203,17 +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
*/
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;
Review comment:
No need to add Collections.unmodifiableSet here as the caller from the
public API will do it.
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Groups.java
##########
@@ -203,17 +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
*/
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;
Review comment:
No need to add Collections.unmodifiableSet here as the caller from the
public API will do it. Will fix in the next commit.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
