[jira] Commented: (HADOOP-6818) Provide a JNI-based implementation of GroupMappingServiceProvider

["Erik Steffl (JIRA)" <ji...@apache.org>]

    [ 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.