Hadoop Common
  1. Hadoop Common
  2. HADOOP-6818

Provide a JNI-based implementation of GroupMappingServiceProvider

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.22.0
    • Component/s: security
    • Labels:
      None

      Description

      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.

      1. JNIGroupMapping.patch
        530 kB
        Devaraj Das
      2. hadoop-6818-1.patch
        532 kB
        Devaraj Das
      3. hadoop-6818-2.patch
        532 kB
        Devaraj Das
      4. 6818-trunk.patch
        526 kB
        Devaraj Das
      5. 6818-trunk-1.patch
        530 kB
        Devaraj Das
      6. 6818-trunk-2.patch
        530 kB
        Devaraj Das

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #412 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk-Commit/412/)

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #412 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk-Commit/412/ )
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #502 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk/502/)
          HADOOP-6818. Provides a JNI implementation of group resolution. Contributed by Devaraj Das.

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #502 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk/502/ ) HADOOP-6818 . Provides a JNI implementation of group resolution. Contributed by Devaraj Das.
          Hide
          Devaraj Das added a comment -

          I committed this. The ReleaseAudit warnings was due to an empty file FTPFileSystemConfigKeys.java in the codebase. The commit for HADOOP-6223 should have taken care of this. I have deleted it in this commit.

          Show
          Devaraj Das added a comment - I committed this. The ReleaseAudit warnings was due to an empty file FTPFileSystemConfigKeys.java in the codebase. The commit for HADOOP-6223 should have taken care of this. I have deleted it in this commit.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12458698/6818-trunk-2.patch
          against trunk revision 1029958.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          -1 release audit. The applied patch generated 3 release audit warnings (more than the trunk's current 1 warnings).

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/11//testReport/
          Release audit warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/11//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/11//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/11//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12458698/6818-trunk-2.patch against trunk revision 1029958. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. -1 release audit. The applied patch generated 3 release audit warnings (more than the trunk's current 1 warnings). +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/11//testReport/ Release audit warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/11//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/11//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/11//console This message is automatically generated.
          Hide
          Erik Steffl added a comment -

          Looks, good, +1

          Show
          Erik Steffl added a comment - Looks, good, +1
          Hide
          Devaraj Das added a comment -

          This should address most of the points raised by Erik. Erik, do you mind taking a quick look please?

          Show
          Devaraj Das added a comment - This should address most of the points raised by Erik. Erik, do you mind taking a quick look please?
          Hide
          Todd Lipcon added a comment -

          Took a look at the updated patch, looks good from my point of view. +1.

          Show
          Todd Lipcon added a comment - Took a look at the updated patch, looks good from my point of view. +1.
          Hide
          Erik Steffl added a comment -

          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)
          Show
          Erik Steffl added a comment - 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)
          Hide
          Devaraj Das added a comment -

          Addresses the comments from Todd.

          Show
          Devaraj Das added a comment - Addresses the comments from Todd.
          Hide
          Todd Lipcon added a comment -

          Hey Devaraj. Thanks for making the changes. One quick one: in the implementation of getGroupForUser() I think it's possible that the cuser memory will be released twice. In the case that getGroupIDList fails, CHECK_ERROR will call ReleaseStringUTFChars, and then the cleanup label will call it again.

          Is it possible to write a junit test case that ensures the results from the JNI mapping are equivalent to the results from the ShellBased mapping? You can use junit's "assume" function so that the tests only run when the native code is available.

          Show
          Todd Lipcon added a comment - Hey Devaraj. Thanks for making the changes. One quick one: in the implementation of getGroupForUser() I think it's possible that the cuser memory will be released twice. In the case that getGroupIDList fails, CHECK_ERROR will call ReleaseStringUTFChars, and then the cleanup label will call it again. Is it possible to write a junit test case that ensures the results from the JNI mapping are equivalent to the results from the ShellBased mapping? You can use junit's "assume" function so that the tests only run when the native code is available.
          Hide
          Devaraj Das added a comment -

          Patch for trunk (incorporates Todd's comments too).

          Show
          Devaraj Das added a comment - Patch for trunk (incorporates Todd's comments too).
          Hide
          Todd Lipcon added a comment -

          Took a look at this patch today, a few notes:

          • In the static initializer of JniBasedUnixGroupsMapping instead of logging Info and throwing an empty RTE, would be nice to include that message as the message of the RTE.
          • In the C code, rather than having a separate function to free things, maybe just use a goto to the end of the function? The forward declaration is a little odd, stylistically.
          • Would be good to add a comment explaining how to run the "test" in getGroup.c, as well
          • in getGroupDetails, probably safer to check if sysconf returns a positive number, and if not set it to something reasonable like 512. Should also probably add sizeof(struct group) to the buffer size here.

          Otherwise looks good. Also +1 on the simplification of the automake structure.

          Show
          Todd Lipcon added a comment - Took a look at this patch today, a few notes: In the static initializer of JniBasedUnixGroupsMapping instead of logging Info and throwing an empty RTE, would be nice to include that message as the message of the RTE. In the C code, rather than having a separate function to free things, maybe just use a goto to the end of the function? The forward declaration is a little odd, stylistically. Would be good to add a comment explaining how to run the "test" in getGroup.c, as well in getGroupDetails, probably safer to check if sysconf returns a positive number, and if not set it to something reasonable like 512. Should also probably add sizeof(struct group) to the buffer size here. Otherwise looks good. Also +1 on the simplification of the automake structure.
          Hide
          Erik Steffl added a comment -

          +1 for hadoop-6818-2.patch

          Show
          Erik Steffl added a comment - +1 for hadoop-6818-2.patch
          Hide
          Devaraj Das added a comment -

          A bit of an update..

          Show
          Devaraj Das added a comment - A bit of an update..
          Hide
          Devaraj Das added a comment -

          Attaching a patch that simplifies the JNI part.

          Show
          Devaraj Das added a comment - Attaching a patch that simplifies the JNI part.
          Hide
          Erik Steffl added a comment -

          +1

          Show
          Erik Steffl added a comment - +1
          Hide
          Devaraj Das added a comment -

          Just a QQ. Is this a duplicate of HADOOP-4998? Or alteast forward compatible?

          No, this is providing a JNI implementation for just the GroupMappingService.

          Show
          Devaraj Das added a comment - Just a QQ. Is this a duplicate of HADOOP-4998 ? Or alteast forward compatible? No, this is providing a JNI implementation for just the GroupMappingService.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Just a QQ. Is this a duplicate of HADOOP-4998? Or alteast forward compatible?

          Lots of ideas floated around there, it'd be good if these two issues are in agreement.

          Show
          Vinod Kumar Vavilapalli added a comment - Just a QQ. Is this a duplicate of HADOOP-4998 ? Or alteast forward compatible? Lots of ideas floated around there, it'd be good if these two issues are in agreement.
          Hide
          Devaraj Das added a comment -

          Patch for Y20S (not for commit). The meat of the patch is in the C and Java files. Also, the src/native/Makefile.am & src/native/configure.ac was changed to easily accommodate this native implementation in the hadoop build.

          Show
          Devaraj Das added a comment - Patch for Y20S (not for commit). The meat of the patch is in the C and Java files. Also, the src/native/Makefile.am & src/native/configure.ac was changed to easily accommodate this native implementation in the hadoop build.

            People

            • Assignee:
              Devaraj Das
              Reporter:
              Devaraj Das
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development