Wangda Tan commented on YARN-2495:

For your comments:
1) For the name, do you think is setAreNodeLabelsUpdated a better name since it 
avoids "set" occured twice :) (I understand this needs lots of refactorings, if 
you have any suggestions, we can finalize it before renaming.

5) I made a mistake that sent an incompleted comment :-p, what I wanted to say 
It will be problematic to ask admins make NM/RM configuration keep 
synchronized, so I don't want (and also not necessary) NM depends on RM's 
So I suggest to make a changes:
- In NodeManager.java: when user doesn't configure provider, it should be null. 
In your patch, you can return a null directly, and YARN-2729 will implement the 
logic of instancing provider from config.
- In NodeStatusUpdaterImpl: avoid using {{isDistributedNodeLabelsConf}}, since 
we will not have "distributedNodeLabelConf" in NM side if you agree on 
previously comment, instead, it will check null of provider.

Regarding your "fail-fast" concern, it shouldn't be a problem if you agree on 
comment I just made. (I know there could be some back-and-forth comment from my 
side on this, I feel sorry about this since this feature is evolving itself, 
please just feel free to let me know your ideas.).

7) I should address your question on 5).

8) You can add an additional comments in line 626 for this.

9) Took a look at TestNodeStatusUpdater, your comment make sense to me, it's a 
very complex class, you can just leave this comment alone.

10) Few comments for your added code:
- updateNodeLabelsInNodeLabelsManager -> updateNodeLabelsFromNMReport
- {{LOG.info(... accepted from RM}}, use LOG.debug and check {{isDebugEnabled}}.
- Make errorMessage clear: indicate 1# this is node labels reported from NM, 
and 2# it's failed to be put to RM instead of "not properly configured".

In addition:
Another thing we should do is, when distributed node label configuration is 
set, any direct modify node to labels mapping from RMAdminCLI should be 
rejected (like -replaceNodeToLabels). This can be done in a separated JIRA.

> Allow admin specify labels from each NM (Distributed configuration)
> -------------------------------------------------------------------
>                 Key: YARN-2495
>                 URL: https://issues.apache.org/jira/browse/YARN-2495
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: resourcemanager
>            Reporter: Wangda Tan
>            Assignee: Naganarasimha G R
>         Attachments: YARN-2495.20141023-1.patch, YARN-2495.20141024-1.patch, 
> YARN-2495.20141030-1.patch, YARN-2495.20141031-1.patch, 
> YARN-2495.20141119-1.patch, YARN-2495.20141126-1.patch, 
> YARN-2495.20141204-1.patch, YARN-2495.20141208-1.patch, 
> YARN-2495.20150305-1.patch, YARN-2495.20150309-1.patch, 
> YARN-2495_20141022.1.patch
> Target of this JIRA is to allow admin specify labels in each NM, this covers
> - User can set labels in each NM (by setting yarn-site.xml (YARN-2923) or 
> using script suggested by [~aw] (YARN-2729) )
> - NM will send labels to RM via ResourceTracker API
> - RM will set labels in NodeLabelManager when NM register/update labels

This message was sent by Atlassian JIRA

Reply via email to