hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Aaron T. Myers (Commented) (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HADOOP-8121) Active Directory Group Mapping Service
Date Fri, 02 Mar 2012 18:53:57 GMT

    [ https://issues.apache.org/jira/browse/HADOOP-8121?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13221148#comment-13221148

Aaron T. Myers commented on HADOOP-8121:

Patch looks pretty good, Natty. I agree the test is substantially improved from the last patch.
A few comments:

# I like the class rename to be more generic, but the class method comment should be changed
to suit.
# Please use 4-space indentation on lines that run over 80 characters. (The configuration
key lines that run over 80 chars are fine.)
# I think it'd be good to have a config prefix variable for "hadoop.ssecurity.group.mapping.ldap"
that you can append all of the config keys to.
# The config keys and their default values should also be put in core-default.xml as well
as in the code, for documentation purposes.
# It seems odd to me that we'll create a new DirContext for every call to getGroups(). Can
that connection to the LDAP server not be cached for the lifetime of the GMSP? Is there a
performance issue with creating a new DirContext each time, e.g. extra round trips to the
LDAP server? (I don't know that there is a perf issue, but there might be.)
# There's a mention of "HDFS" in the comments of LdapGroupsMapping, but the class will be
used by both HDFS and MR.
# Are there not constants in the Java libraries that could be used in lieu of the hard-coded
strings "javax.net.ssl.keyStorePassword", "javax.net.ssl.keyStore", etc? (There very well
may not be, I'm not sure.)
# Using the mockContext from a non-static inner class seems a little goofy to me. Instead,
try just making an instance of LdapGroupsMapping and then using Mockito.spy(...) to interpose
on the calls to createDirContext.
# Add an "ldapUrl == null ||" to the check in setConf for an unconfigured ldapUrl.
# You might consider a static import of Mockito.*, so you can get rid of all the "Mockito."
throughout the test.
# Some goofy indentation in the first call to "Mockito.when".
# The test class could use a few more comments, e.g. it took me a minute to realize you were
setting up the mock to return first the user name, then the group name on consecutive calls
to DirContext#search.
> Active Directory Group Mapping Service
> --------------------------------------
>                 Key: HADOOP-8121
>                 URL: https://issues.apache.org/jira/browse/HADOOP-8121
>             Project: Hadoop Common
>          Issue Type: New Feature
>          Components: security
>            Reporter: Jonathan Natkins
>            Assignee: Jonathan Natkins
>         Attachments: HADOOP-8121.2.patch, HADOOP-8121.3.patch, HADOOP-8121.patch
> Planning on building a group mapping service that will go and talk directly to an Active
Directory setup to get group memberships

This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira


View raw message