HADOOP-13103 Group resolution from LDAP may fail on javax.naming.ServiceUnavailableException

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

@@ -18,15 +18,14 @@
 package org.apache.hadoop.security;
 
 import java.io.FileInputStream;
-import java.io.FileReader;
 import java.io.IOException;
 import java.io.InputStreamReader;
 import java.io.Reader;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.Hashtable;
 import java.util.List;
 
-import javax.naming.CommunicationException;
 import javax.naming.Context;
 import javax.naming.NamingEnumeration;
 import javax.naming.NamingException;
@@ -43,7 +42,6 @@ import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.classification.InterfaceStability;
 import org.apache.hadoop.conf.Configurable;
 import org.apache.hadoop.conf.Configuration;
-import org.apache.hadoop.io.IOUtils;
 
 /**
  * An implementation of {@link GroupMappingServiceProvider} which
@@ -197,7 +195,7 @@ public class LdapGroupsMapping
   private String posixGidAttr;
   private boolean isPosix;
 
-  public static int RECONNECT_RETRY_COUNT = 3;
+  public static final int RECONNECT_RETRY_COUNT = 3;
   
   /**
    * Returns list of groups for a user.
@@ -210,40 +208,26 @@ public class LdapGroupsMapping
    * @return list of groups for a given user
    */
   @Override
-  public synchronized List<String> getGroups(String user) throws IOException {
+  public synchronized List<String> getGroups(String user) {
-    List<String> emptyResults = new ArrayList<String>();
     /*
      * 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. 
      */
+    for(int retry = 0; retry < RECONNECT_RETRY_COUNT; retry++) {
       try {
         return doGetGroups(user);
-    } catch (CommunicationException e) {
-      LOG.warn("Connection is closed, will try to reconnect");
       } catch (NamingException e) {
-      LOG.warn("Exception trying to get groups for user " + user + ": "
+        LOG.warn("Failed to get groups for user " + user + " (retry=" + retry
-          + e.getMessage());
+            + ") by " + e);
-      return emptyResults;
+        LOG.trace("TRACE", e);
       }
 
-    int retryCount = 0;
-    while (retryCount ++ < RECONNECT_RETRY_COUNT) {
       //reset ctx so that new DirContext can be created with new connection
       this.ctx = null;
-      
-      try {
-        return doGetGroups(user);
-      } catch (CommunicationException e) {
-        LOG.warn("Connection being closed, reconnecting failed, retryCount = " + retryCount);
-      } catch (NamingException e) {
-        LOG.warn("Exception trying to get groups for user " + user + ":"
-            + e.getMessage());
-        return emptyResults;
-      }
     }
     
-    return emptyResults;
+    return Collections.emptyList();
   }
   
   List<String> doGetGroups(String user) throws NamingException {
@@ -297,6 +281,9 @@ public class LdapGroupsMapping
       }
     }
 
+    if (LOG.isDebugEnabled()) {
+      LOG.debug("doGetGroups(" + user + ") return " + groups);
+    }
     return groups;
   }
 

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

@@ -1601,9 +1601,11 @@ public class UserGroupInformation {
       return result.toArray(new String[result.size()]);
     } catch (IOException ie) {
       if (LOG.isDebugEnabled()) {
-        LOG.debug("No groups available for user " + getShortUserName());
+        LOG.debug("Failed to get groups for user " + getShortUserName()
+            + " by " + ie);
+        LOG.trace("TRACE", ie);
       }
-      return new String[0];
+      return StringUtils.emptyStringArray;
     }
   }
   

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

@@ -85,7 +85,7 @@ public class TestLdapGroupsMapping extends TestLdapGroupsMappingBase {
     
     // Ldap server is down, no groups should be retrieved
     doTestGetGroups(Arrays.asList(new String[] {}), 
-        1 + LdapGroupsMapping.RECONNECT_RETRY_COUNT); // 1 is the first normal call
+        LdapGroupsMapping.RECONNECT_RETRY_COUNT);
   }
   
   private void doTestGetGroups(List<String> expectedGroups, int searchTimes) throws IOException, NamingException {