Wangda Tan commented on YARN-3075:

Varun, Thanks for working on this, and also thanks for comments from Sunil, 
some comments in my side:

First I think I should brief how current CommonsNodeLabelsManager works for 
multiple NMs running on a node, just in case if you have any misunderstanding 
about it.

Host -> Node mappings looks like:

   Host1       Host2            Host3
  /   \          |           /    |    \ 
 NM1.1 NM1.2     NM2        NM3.1 NM3.2 NM3.3

The reason why there's "host" layer because multiple NMs can run on a same host 
(it is not a typical YARN model, but YARN supports that), and user can specify 
labels on individual NMs.

If admin specifies labels on a host (using "host" or "host:0"), labels of all 
NMs on it will get updated altogether. When user tries to get labels on a NM, 
if Node.labels == null), labels on Host will be returned, other wise, 
Node.labels will be returned.

*Back to your patch:*
1) When op (add/remove/replace) is on a host {{nodeId.getPort() == 
WILDCARD_PORT}}, (of course you need update label->host), you only need update 
label->Nodes when check {{node.labels != null}} is true.
2) When op is on a node, as mentioned by Sunil, replace opertions not correct, 
it should be remove and then add. 
3) {{getLabelsToNodes}}:
3.1 Two loop seems duplicated, you can set labels = labelCollections.entrySet 
when (labels == null or empty).
3.2 When labels == null or empty, it will returns nodes on all labels. You need 
add a javadocs to brief this behavior and you need remove empty label from 
labelCollection like what we did in {{getClusterNodeLabels}}. The "empty" label 
exists because we need track non-labeled nodes in scheduler side but it 
shouldn't be seen by user.
3.3 When a label contains (nodeId.port = WILDCARD_PORT), you should add Nodes 
in the host if (node.labels == null). It is possible a. admin specify 
host1.label = x; b. nm1 on host1 activated. You should get nm1 when you inquire 
nodes of label=x. You may need to add a test to TestRMNodeLabelsManager. You 
can take a look at {{testNodeActiveDeactiveUpdate}}
4) In add(remove/replace)NodeToLabels, such null check is not necessary: if 
(label != null). It will be checked in check... methods in 

May have more comments in next iteration :).


> NodeLabelsManager implementation to retrieve label to node mapping
> ------------------------------------------------------------------
>                 Key: YARN-3075
>                 URL: https://issues.apache.org/jira/browse/YARN-3075
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: resourcemanager
>    Affects Versions: 2.7.0
>            Reporter: Varun Saxena
>            Assignee: Varun Saxena
>         Attachments: YARN-3075.001.patch

This message was sent by Atlassian JIRA

Reply via email to