[jira] [Commented] (YARN-5764) NUMA awareness support for launching containers

["Miklos Szegedi (JIRA)" <ji...@apache.org>]

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

Miklos Szegedi commented on YARN-5764:
--------------------------------------

Thank you for the patch, [~devaraj.k].
{{yarn.nodemanager.numa-awareness.node-ids}} might need a good example in the documentation.
{code}
409	    // Intialize Numa Resource Allocator
410	    if (YarnConfiguration.numaAwarenessEnabled(conf)) {
411	      LocalResourceAllocators
412	          .setNumaResourceAllocator(new NumaResourceAllocator(context));
413	    }
{code}
It might be useful to add a LOG.info of the fact that NUMA is enabled.
{code}
55	    ADD_NUMA_PARAMS("");
{code}
It is missing a comment {{//no CLI switch supported yet.}}
{code}
LocalResourceAllocators
{code}
Does it make sense to have this class to contain a static, instead of a simple static field in NumaResourceHandlerImpl?
Public methods in NumaNodeResource.assignAvailableMemory need a javadoc.
NumaNodeResource.recoverMemory and recoverCpus are not used.
Containers can change their resource usage. I do not see that supported, yet. It may need another jira.
{code}
177	          if (GB.equals(units)) {
178	            memory = memory * 1024;
179	          } else if (KB.equals(units)) {
180	            memory = memory / 1024;
181	          }
{code}
Is MB not supported?
{code}
103	          for (int nodeId = Integer.parseInt(beginNEnd[0]); nodeId <= Integer
104	              .parseInt(beginNEnd[1]); nodeId++) {
{code}
You might want to handle the parsing outside the loop.
{code}
146	    String cmdOutput = shExec.getOutput();
147	    return cmdOutput;
{code}
cmdOuput is redundant. You could use just one line.
{code}
203	   * @param resource resource for the Numa Node
204	   * @return the assigned NUMA Node info or null if resources not available.
{code}
I see a casting mismatch of NUMA. Also, do we need the commented code in the function body?
I think noOfNumaNodes is redundant. Could we just use numaNodesList.size();?
I see {{numaNode.assignAvailableMemory}} called and then {{releaseNumaResource(containerId)}}, if it did not allocate enough memory. I am wondering if it would be simpler not to allocate in this case?
{code}
StringBuilder memNodes = new StringBuilder();
StringBuilder cpuNodes = new StringBuilder();
{code}
These two are not populated.
{code}
  public synchronized void recoverNumaResource(ContainerId containerId) {
{code}
I see commented code in the body of the function and also in the unit tests.
is package-info.java necessary? 

> NUMA awareness support for launching containers
> -----------------------------------------------
>
>                 Key: YARN-5764
>                 URL: https://issues.apache.org/jira/browse/YARN-5764
>             Project: Hadoop YARN
>          Issue Type: New Feature
>          Components: nodemanager, yarn
>            Reporter: Olasoji
>            Assignee: Devaraj K
>         Attachments: NUMA Awareness for YARN Containers.pdf, NUMA Performance Results.pdf, YARN-5764-v0.patch, YARN-5764-v1.patch, YARN-5764-v2.patch, YARN-5764-v3.patch, YARN-5764-v4.patch, YARN-5764-v5.patch
>
>
> The purpose of this feature is to improve Hadoop performance by minimizing costly remote memory accesses on non SMP systems. Yarn containers, on launch, will be pinned to a specific NUMA node and all subsequent memory allocations will be served by the same node, reducing remote memory accesses. The current default behavior is to spread memory across all NUMA nodes.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org