hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Erik Steffl (JIRA)" <j...@apache.org>
Subject [jira] Commented: (HADOOP-6818) Provide a JNI-based implementation of GroupMappingServiceProvider
Date Wed, 13 Oct 2010 23:16:34 GMT

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

Erik Steffl commented on HADOOP-6818:
-------------------------------------

6818-trunk-1.patch review, nothing major, just few details to consider.

  - line length over 80 chars, mismatched intendation in few cases (e.g. JniBasedUnixGroupsMapping.java
line: "be loaded");) etc., not sure how much we care about detials like that

  - TestJNIGroupsMapping.java - test compares groups returned by ShellBasedUnixGroupsMapping
and JniBasedUnixGroupsMapping, first it compares group lists for size then it looks up whether
all groups returned by ShellBasedUnixGroupsMapping are also in JniBasedUnixGroupsMapping.
This comparison would fail for lists like (a, a, b) and (a, b, c). It's very unlikely that
ShellBasedUnixGroupsMapping would list the same group twice but it's fairly cheap to do exact
comparison (i.e. sort the lists and compare the items one by one)

  - JniBasedUnixGroupsMapping.java getGroups: is it not possible to return empty array from
getGroupForUser and get rid of if (groups != null && groups.length != 0) and the rest
of the function?

  - configure: what's the point of (unset CDPATH)? As far as I can tell it creates a subshell
in which it unsets CDPATH which has no effect in current shell. I see the result is used to
determine whether unset CDPATH is executed but why? Given that it's not clear maybe a comment
would help. (or is this file generated?)

  - JniBasedUnixGroupsMapping.c: it looks like code would be a bit simpler of cleanup label
did CHECK_ERROR and code that jumps to cleanup just set the error. If cuser is NULL it could
also jump to cleanup. That way there would be only ene exit point of the function, one place
responsible for resource cleanup etc.

  - getGroup.c: functions in this file do lot of pointer manipulation, it seems they would
also benefit from same techinzue used in JniBasedUnixGroupsMapping.c, i.e. initialize all
pointers to NULL, in case of error jump to cleanup label that cleans everything up. Would
free maintainer from thinking about whether pwbuf or group (or both or neither) should be
freed or not etc.

  -  getGroup.c getGroupIDList: in case of error ngroup is not reset to zero, shouldn't matter
but would be more consistent (i.e. ngroups should always be same as number of groups in groups)

> Provide a JNI-based implementation of GroupMappingServiceProvider
> -----------------------------------------------------------------
>
>                 Key: HADOOP-6818
>                 URL: https://issues.apache.org/jira/browse/HADOOP-6818
>             Project: Hadoop Common
>          Issue Type: Improvement
>          Components: security
>            Reporter: Devaraj Das
>            Assignee: Devaraj Das
>             Fix For: 0.22.0
>
>         Attachments: 6818-trunk-1.patch, 6818-trunk.patch, hadoop-6818-1.patch, hadoop-6818-2.patch,
JNIGroupMapping.patch
>
>
> The default implementation of GroupMappingServiceProvider does a fork of a unix command
to get the groups of a user. Since the group resolution happens in the servers, this might
be costly. This jira aims at providing a JNI-based implementation for GroupMappingServiceProvider.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message