hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Colin Patrick McCabe (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HADOOP-10527) Fix incorrect return code and allow more retries on EINTR
Date Mon, 21 Apr 2014 22:33:27 GMT

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

Mime
View raw message