[
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: [email protected]
For additional commands, e-mail: [email protected]
