Karthik Kambatla commented on YARN-1012:

Thanks for updating the patch, Inigo. Looks better, few minor comments:
# ResourceUtilization of containers is calculated only when container 
monitoring is enabled. Given the low overhead of sending this across, I think 
it is okay to not add another config to choose whether to send this information 
across or not. However, if container-monitoring is not enabled, we should 
probably not send it across. Accordingly, would it make sense to not modify 
{{NodeStatus#newInstance}} and just call the setter when required.
# ResourceUtilization
## javadoc has an error
 * @see Resource
## Now that we have moved it to be server-side only, does it need to Public? 
How about changing it to Private-Evolving
## We don't need to add annotations for individual methods if it is the same as 
the enclosing class. 
# ContainersMonitorImpl - Extra space below 
         // Save the aggregated utilization of the containers
        setContainersUtilization(trackedContainersUtilization );
# NodeStatus doesn't need to ResourceUtilization
# NodeStatusUpdaterImpl doesn't need to import Records

> Report NM aggregated container resource utilization in heartbeat
> ----------------------------------------------------------------
>                 Key: YARN-1012
>                 URL: https://issues.apache.org/jira/browse/YARN-1012
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: nodemanager
>    Affects Versions: 2.7.0
>            Reporter: Arun C Murthy
>            Assignee: Inigo Goiri
>         Attachments: YARN-1012-1.patch, YARN-1012-2.patch, YARN-1012-3.patch, 
> YARN-1012-4.patch, YARN-1012-5.patch, YARN-1012-6.patch, YARN-1012-7.patch, 
> YARN-1012-8.patch, YARN-1012-9.patch

This message was sent by Atlassian JIRA

Reply via email to