hadoop-hdfs-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] (HDFS-4712) New libhdfs method hdfsGetDataNodes
Date Tue, 23 Jul 2013 17:42:49 GMT

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

Colin Patrick McCabe commented on HDFS-4712:
--------------------------------------------

{code}
+    if (jVal.l == NULL) 
+        strcpy(dataNodeInfos->hostName,"");
+    else 
+        jExc =  newCStr(env, jVal.l, &dataNodeInfos->hostName);
{code}

You want {{dataNodeInfos->hostName = strdup("")}} here

{code}
+    typedef struct  hdfsDataNodeInfo_s {
+        long capacity;        /* The raw capacity */
+        long dfsUsed;         /* The used space by the data node*/
+        char *hostName;       /* HostName as supplied by the datanode during registration
as its name*/
+        tTime lastUpdate;     /* The time when this information was accurate */
+        char *location;       /* The rack name */
+        long remaining;       /* The raw free space */
+        short xceiverCount;   /* The  number of active connections */
+    } hdfsDataNodeInfo;
{code}

Please use {{int64_t}} instead of {{long}} here.  {{long}} has different sizes on different
architectures, but these quantities should always be 64-bit.  I think it makes more sense
to use {{uint32_t}} for xceiverCount, since I don't know if "32767 will be enough for anyone."

{code}
+         if (hdfsDataNodeInfos[i]->hostName) {
+            free(hdfsDataNodeInfos[i]->hostName);
+          }
+         if (hdfsDataNodeInfos[i]->location) {
+            free(hdfsDataNodeInfos[i]->location);
+        }
{code}
You don't need to test if something is NULL before calling {{free}} on it.  {{free}} ignores
NULL pointers.

{{hdfsGetDataNodeInfo}}: the error handling is wrong here.  You should return NULL when there
is an error.  You must also free the memory you've allocated on this line:
{code}
+    dataNodeInfoList = calloc(jNumDataNodeInfos, sizeof(hdfsDataNodeInfo*));
{code}

It is preferrable to use a pattern like this:
{code}
  if (bad thing) {
    ret = EIO;
    goto done;
  }

done:
  if (ret) {
    free(resources);
    resources = NULL;
    errno = ret;
  }
  return resources;
{code}

This way:
* You always free the resources on error.
* setting errno is the last thing you do, and won't be overwritten by another function call.
* You return NULL on error rather than a bogus pointer.

thanks
                
> New libhdfs method hdfsGetDataNodes
> -----------------------------------
>
>                 Key: HDFS-4712
>                 URL: https://issues.apache.org/jira/browse/HDFS-4712
>             Project: Hadoop HDFS
>          Issue Type: New Feature
>          Components: libhdfs
>            Reporter: andrea manzi
>         Attachments: HDFS-4712.patch, hdfs.c.diff, hdfs.h.diff
>
>
> we have implemented a possible extension to libhdfs to retrieve information about the
available datanodes ( there was a mail on the hadoop-hdsf-dev mailing list initially abut
this :
> http://mail-archives.apache.org/mod_mbox/hadoop-hdfs-dev/201204.mbox/%3CCANhO-
> s0mvORORrxPjnjbQL6bRkJ4C7L+u816xkdc+2r0WHjBcw@mail.gmail.com%3E)
> i would like to know how to proceed to create a patch, cause on the wiki http://wiki.apache.org/hadoop/HowToContribute
i can see info about JAVA patches but nothing related to extensions in C.

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

Mime
View raw message