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

        

Mime
View raw message