[
https://issues.apache.org/jira/browse/HADOOP-9439?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13677534#comment-13677534
]
Colin Patrick McCabe commented on HADOOP-9439:
----------------------------------------------
Thanks, very thorough review.
I fixed the naming of the parameters to getGroupsForUser and logError.
Although it didn't affect correctness, it certainly was very confusing.
You're right that {{GetStaticMethodID}} and {{FindClass}} throw exceptions on
failure. There is no need to throw another one (and it is probably actually
harmful). Thanks for finding that. However, {{NewGlobalRef}} does not throw
an exception, but merely returns {{NULL}} when you're out of memory.
I will add a comment clarifying why we ignore exceptions in logError.
I agree that we should probably just use the existing monitor lock from
NativeIO.java. That way, I don't have to modify that code. I wasn't aware
that there were other folks poking the user/group functions in Hadoop. Right
now, it looks like {{NativeIO#getUserName}} is only called from tests calling
{{NativeIO#POSIX#getFstat}}, but of course that may change in the future.
Invalid groups are a sore point for {{ShellBasedUnixGroupsMapping}}. If any
invalid groups are associated with a user, the "{{groups}}" program will fail
with a non-zero return code, and no information is returned. For
{{JniBasedUnixGroupsMapping}}, I would prefer to return the groups that were
valid, rather than nothing at all. I suppose this is debatable, though. I can
test creating such invalid groups.
I understand that sometimes it's unnecessary, but I'd rather have
{{DeleteLocalRef}} used for all allocations. For one thing, in libhdfs, it
really *is* necessary everywhere (the JNI invocation API never automatically
disposes of Java references that are made in the invoking C code). It confuses
people when they copy a piece of code from one part of the source tree to
another and it suddenly becomes incorrect. For another thing, the spec only
says that the JVM has to provide at least 16 local references at once, which is
not very many at all. It's only one line of overhead per Java reference, and
{{DeleteLocalRef}} already ignores NULLs, so I'd rather just have a consistent
style everywhere, than try to be clever.
> JniBasedUnixGroupsMapping: fix some crash bugs
> ----------------------------------------------
>
> Key: HADOOP-9439
> URL: https://issues.apache.org/jira/browse/HADOOP-9439
> Project: Hadoop Common
> Issue Type: Bug
> Components: native
> Affects Versions: 2.0.4-alpha
> Reporter: Colin Patrick McCabe
> Assignee: Colin Patrick McCabe
> Priority: Minor
> Attachments: HADOOP-9439.001.patch, HADOOP-9439.003.patch,
> HDFS-4640.002.patch
>
>
> JniBasedUnixGroupsMapping has some issues.
> * sometimes on error paths variables are freed prior to being initialized
> * re-allocate buffers less frequently (can reuse the same buffer for multiple
> calls to getgrnam)
> * allow non-reentrant functions to be used, to work around client bugs
> * don't throw IOException from JNI functions if the JNI functions do not
> declare this checked exception.
> * don't bail out if only one group name among all the ones associated with a
> user can't be looked up.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
