Vinod Kumar Vavilapalli commented on YARN-1856:

Quick comments on the patch:

 - Should add all the configs to yarn-default.xml, saying they are still early 
 - Should update the documentation of pmem-check-enabled, vmem-check-enabled 
configs in code and yarn-default.xml to denote their relation to 
 - Actually, given existing memory monitoring mechanism, 
NM_MEMORY_RESOURCE_ENABLED is in reality is already true when pmem/vmem checks 
are enabled. We need to reconcile the old and new configs some how. May be 
memory is always enabled, but if vmem/pmem configs are enabled, use old 
handler, otherwise use the new one? Thinking out aloud.
 - Does the soft and hard limits also some-how logically relate to 
pmem-vmem-ratio? If so, we should hint at that in the documentation.
 - Swappiness seems like a cluster configuration defaulting to zero. So far, 
this has been an implicit contract with our users, good to document this also 
in yarn-default.xml

Code comments
 - ResourceHandlerModule
    -- Formatting of new code is a little off: the declaration of 
{{getCgroupsMemoryResourceHandler()}}. There are other occurrences like this in 
that class before in this patch, you may want to fix those.
    -- BUG! getCgroupsMemoryResourceHandler() incorrectly locks 
DiskResourceHandler instead of MemoryResourceHandler.
 - CGroupsMemoryResourceHandlerImpl
    -- What is this doing? {{  CGroupsHandler.CGroupController MEMORY =    
CGroupsHandler.CGroupController.MEMORY; }} Is it forcing a class-load or 
something? Not sure if this is needed. If this is needed, you may want to add a 
comment here.
/ CGROUP_PARAM_MEMORY_SWAPPINESS can all be static and final.

> cgroups based memory monitoring for containers
> ----------------------------------------------
>                 Key: YARN-1856
>                 URL: https://issues.apache.org/jira/browse/YARN-1856
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: nodemanager
>    Affects Versions: 2.3.0
>            Reporter: Karthik Kambatla
>            Assignee: Varun Vasudev
>         Attachments: YARN-1856.001.patch, YARN-1856.002.patch, 
> YARN-1856.003.patch

This message was sent by Atlassian JIRA

Reply via email to