hadoop-yarn-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] (YARN-147) Add support for CPU isolation/monitoring of containers
Date Wed, 17 Oct 2012 17:00:04 GMT

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

Colin Patrick McCabe commented on YARN-147:
-------------------------------------------

Thanks for working on this, Andrew!  I took a quick look at the native part of this patch.

{{get_kv_key}}, {{get_kv_value}}: I would recommend that your API look something like this:
{code}
/**
 * If str is a string of the form key=val, find 'key'
 * 
 * @param input    The input string
 * @param out      Where to put the output string.
 * @param out_len  The length of the output buffer.
 *
 * @return         -ENAMETOOLONG if out_len is not long enough;
 *                 -EINVAL if there is no equals sign in the input;
 *                 0 on success
 */
int get_key(const char *input, char *out, size_t out_len);
{code}

This API doesn't require you to modify the input or allocate memory-- two things that make
the code very hard to follow.  It gives the caller the choice of how much memory to allocate
and how to allocate it (stack, malloc, etc.)

{code}
  char pid_buf[21];
  snprintf(pid_buf, 21, "%d", pid);
{code}

{{snprintf(pid_buf, sizeof(pid_buf), "%d", pid);}} would be considered better style.

More importantly, it would probably be better for you to use {{fopen}} / {{fprintf}} / {{fclose}}
here.  If you do want to use the raw {{open}} / {{write}} / {{close}} methods, you have to
correctly handle short writes and {{EINTR}}, which you are not doing here.

{{mount_cgroup}}: you really need to check the return code of {{mount}} and do something if
it fails.

I would also recommend that you allocate {{hier_path}} on the stack with 
{code}
char hier_path[PATH_MAX]
{code}
or similar.  Using {{malloc}} here is not really worth it.

{code}
stpncpy(buf, hierarchy, strlen(hierarchy));
{code}

This seems to do the same thing here as the more widely available and portable {{strncpy}}
function.  (You're not using the pointer which {{stpncpy}} returns.)  Better yet, use {{snprintf(buf,
sizeof(buf), "%s", hierarchy);}} which will do bounds checking properly.

{{main.c}}: Just a general comment.  I really recommend using {{getopt_long}} to parse options.
 I've seen manual option parsing grow into a huge mess over time as everyone adds "just one
more option."  You don't have to do that in this change (although it would be easy enough
to do), but please consider it in the future.
                
> Add support for CPU isolation/monitoring of containers
> ------------------------------------------------------
>
>                 Key: YARN-147
>                 URL: https://issues.apache.org/jira/browse/YARN-147
>             Project: Hadoop YARN
>          Issue Type: Bug
>          Components: nodemanager
>    Affects Versions: 2.0.3-alpha
>            Reporter: Alejandro Abdelnur
>            Assignee: Andrew Ferguson
>             Fix For: 2.0.3-alpha
>
>         Attachments: YARN-147-v1.patch, YARN-147-v2.patch, YARN-3.patch
>
>
> This is a clone for YARN-3 to be able to submit the patch as YARN-3 does not show the
SUBMIT PATCH button.

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