Hexiaoqiao commented on a change in pull request #2085:
URL: https://github.com/apache/hadoop/pull/2085#discussion_r444638974
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/JniBasedUnixGroupsMapping.java
##########
@@ -19,9 +19,9 @@
package org.apache.hadoop.security;
import java.io.IOException;
-import java.util.Arrays;
-import java.util.List;
+import java.util.*;
Review comment:
suggest single class import.
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Groups.java
##########
@@ -345,28 +373,28 @@ public long read() {
* 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>>() {
Review comment:
replace with lambda statement?
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/NullGroupsMapping.java
##########
@@ -31,6 +33,19 @@
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 null;
Review comment:
return Collections.emptySet();
return EMPTY set rather than Null may be more safety?
##########
File path:
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSPermissionChecker.java
##########
@@ -549,7 +549,6 @@ private boolean hasPermission(INodeAttributes inode,
FsAction access) {
* - Default entries may be present, but they are ignored during enforcement.
*
* @param inode INodeAttributes accessed inode
- * @param snapshotId int snapshot ID
Review comment:
It seems that is not related to this changes.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterUserMappings.java
##########
@@ -111,6 +112,11 @@ public void cacheGroupsRefresh() throws IOException {
@Override
public void cacheGroupsAdd(List<String> groups) throws IOException {
}
+
+ @Override
+ public Set<String> getGroupsSet(String user) throws IOException {
+ return null;
Review comment:
Do we need keep the same logic with getGroups. Not sure if this method
will invoke by other unit test.
```
@Override
public List<String> getGroups(String user) throws IOException {
LOG.info("Getting groups in MockUnixGroupsMapping");
String g1 = user + (10 * i + 1);
String g2 = user + (10 * i + 2);
List<String> l = new ArrayList<String>(2);
l.add(g1);
l.add(g2);
i++;
return l;
}
```
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-httpfs/src/test/java/org/apache/hadoop/lib/service/security/DummyGroupMapping.java
##########
@@ -47,4 +48,9 @@ public void cacheGroupsRefresh() throws IOException {
@Override
public void cacheGroupsAdd(List<String> groups) throws IOException {
}
+
+ @Override
+ public Set<String> getGroupsSet(String user) throws IOException {
+ return null;
Review comment:
`return Collections.emptySet(); `?
BTW, it seems that class `DummyGroupMapping` is never used now, do we need
scrubbed it off?
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/authorize/AccessControlList.java
##########
@@ -20,10 +20,7 @@
import java.io.DataInput;
import java.io.DataOutput;
import java.io.IOException;
-import java.util.Collection;
-import java.util.HashSet;
-import java.util.LinkedList;
-import java.util.List;
+import java.util.*;
Review comment:
another one star import.
----------------------------------------------------------------
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
