Naganarasimha G R updated YARN-2495:
    Attachment: YARN-2495.20150309-1.patch

Hi [~wangda], 
Attaching the updated patch and find the status of the comments :
1,2,3,4 : all have been rectified, method name having "set" in the begin and 
the end was not sounding appropriate, hence modified it to 
areNodeLabelsSetInReq and setAreNodeLabelsSetInReq  in both heartbeat and 

5) ??I think we may not need to check centralized/distributed configuration 
here, centralized/distributed is a config in RM side.??
   Earlier had moved this configuration type check in the NM.serviceInit (l) 
before calling {{getNodeLabelsProviderService}} But now changed the method name 
to {{createNodeLabelsProviderService}} and moved back the check inside this 
method itself. As part of the YARN-2729 will be returning Script based node 
label provider in the createNodeLabelsProviderService method.

??In NM side, it should be how to get node labels, if user doesn't configure 
any script file for it, it should be null and no instance of 
NodeLabelProviderService will be added to NM.??
In current patch, null will be set only if configuration type is set as 
centralized in NM and based on earlier(other jira) feedback from Vinod, i think 
we need to fail fast and let the user know the error at the earliest, so script 
node label provider will throw exception on erroneous conditions like script 
not configured,no rights to execute etc.. and ensure NM will fail to start. 

??So back to code, you can just leave getNodeLabelsProviderService(..), which 
will be implemented in YARN-2729.If you agree, we need change the name 
isDistributedNodeLabelsConf to??
Actually dint get the intent of these 2 lines and felt like comment was not 
complete... Is it you want to avoid check of configuration type in NM and move 
it script node label provider or something ?

6) has been rectified, was added while analyzing test case failure.

7) ??isDistributedNodeLabels seems not so necessary here, and if you agree with 
5), it's better to remove the field??
IIUC point5 was related to NM initializing the provider and point7 is related 
to NodeStatusUpdaterImpl if so i dint get the relation.  can you please clarify 
these 2 points 

8) ??Add null check or comment (provider returned node labels will always be 
not-null, for areNodeLabelsUpdated in NodeStatusUpdaterImpl??
Before calling areNodeLabelsUpdated, i had already checked for null and set 
empty labels @ line 626 (startStatusUpdater method) 

9)??Since we already have TestNodeStatusUpdater, it's better to merge 
TestNodeStatusUpdaterForLabels to it.??
Well there was already too many internal classes extending 
NodeStatusUpdaterImpl and ResourceTrackerService. And personally felt very very 
difficult to walk through the test case and try to reuse it and class had 
already crossed 1666 lines of code and hence as it was loosing readability 
added a new class. Please inform if required will merge it to the existing 
class only

10) Have modified ResourceTrackerService based on ur comments and have pushed 
some common code in register and Heartbeat to the common method.

All findbugs issues are not related to my modifications and following test case 
failure is not related to my modification

Also will be uploading a patch for 2729 to get the view of complete flow and 
also should will be testable

> 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