[
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
