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

Colin Patrick McCabe commented on HADOOP-10527:
-----------------------------------------------

Kihwal wrote:

bq. I don't think it is correct to throw a RuntimeException when a user is not 
found (ENOENT).

We can't throw an IOException because the JNI function does not declare 
"{{throws IOException}}"  Throwing an exception not in the throw spec causes 
undefined behavior.  If you want to change the throw spec, that would also work.

You can see in JniBasedUnixGroupsMapping.java that the JNI function is declared 
like this:
{code}
  native static String[] getGroupsForUser(String username);
{code}

(There is no "{{throws IOException}}")

bq. The patch does not use errno. It explicitly use the return value. The code 
before my patch used errno.

OK, I missed the part where you removed this line:
{code}
-    } while ((!pwd) && (errno == EINTR));
{code}

That was indeed a good line to remove.

However, your patch adds these lines:
{code}
+    // The following call returns errno. Reading the global errno wihtout
+    // locking is not thread-safe.
{code}

This is doubly wrong because
* {{getpwnam_r}} doesn't set errno -- it returns the error code as a result
* {{errno}} is not global and is always thread-safe

So we need to remove this comment.

I think we need to get rid of {{MAX_USER_LOOKUP_TRIES}} and replace it with a 
maximum buffer size that we don't go over when we get {{ERANGE}}.  The problem 
with the current scheme is that it doesn't actually put a limit on buffer size 
since we don't consider this in our decision to multiply the buffer size by 2x 
or not.

{code}
+      char buf[128];
+      snprintf(buf, sizeof(buf), "getgrouplist: error looking up user. %d 
(%s)",
+               ret, terror(ret));
+      THROW(env, "java/lang/RuntimeException", buf);
{code}

Should use either {{newRuntimeException}} or {{newIOException}} here.

> Fix incorrect return code and allow more retries on EINTR
> ---------------------------------------------------------
>
>                 Key: HADOOP-10527
>                 URL: https://issues.apache.org/jira/browse/HADOOP-10527
>             Project: Hadoop Common
>          Issue Type: Bug
>            Reporter: Kihwal Lee
>            Assignee: Kihwal Lee
>         Attachments: hadoop-10527.patch
>
>
> After HADOOP-10522, user/group look-up will only try up to 5 times on EINTR.  
> More retries should be allowed just in case.
> Also, when a user/group lookup returns no entries, the wrapper methods are 
> returning EIO, instead of ENOENT.



--
This message was sent by Atlassian JIRA
(v6.2#6252)

Reply via email to