Naganarasimha G R commented on YARN-2495:

Thanks for reviewing Wangda :
bq. 2) It seems NM_LABELS_FETCH_INTERVAL_MS not been used in the patch, did you 
forget to do that?
-- Earlier was planning to make node labels script only to be dynamic and 
configruation based as static. Now based on your comment 4 will make it dynamic 
and change the configuration name too.

bq. 3) Regarding ResourceTrackerProtocol, I think NodeHeartbeatRequest should 
only report labels when labels changed. So there're 3 possible values of node 
labels in NodeHeartbeatRequest ... And RegisterNodeManagerRequest should report 
label every time registering.
-- Yes this was my plan and will be doing it in the same way. 
But was thinking about one sceanario labels got changed and on call to 
NodeLabelsProvider.getLabels() it returns the new labels but the heartbeat 
failed due to some reason. in that case NodeLabelsProvider will not be able to 
detect this and on next request to  getLabels() it will return null. So we 
should have some mechanism such that NodeLabelsProvider are informed whether RM 
accepted the change in labels so that appropriate SET of labels are provided on 
call to getLabels (also if needed we can have RM Rejected Labels too for 
logging purpose)
Planning to have 3 interfaces in NodeLabelsProvider
        * getNodeLabels() : to get the labels which can be used for 
        * getNodeLabelsOnModify() :  to get the labels on modification which 
can be used for heartbeat
        * rmUpdateNodeLabelsStatus(boolean success) : to indicate that next 
call to getNodeLabelsOnModify can be reset to null

bq. 4.1 Why this class extends from CompositeService? Did you want to add more 
component to it? If not, AbstractService should be enough. If the purpose of 
the NodeLabelsFetcherService is only create a NodeLabelsProvider, and the 
NodeLabelsProvider will take care of periodically read configuration from 
yarn-site.xml.I suggest to rename NodeLabelsFetcherService to 
NodeLabelsProviderFactory, and not extends from any Service, because the 
NodeLabelsProvider should be a Service. Rename NodeLabelsProvider to 
NodeLabelsProviderService if your purpose is as what I mentioned.
        -- Your idea seems to be better, will try to do it in the way you have 
specified and hence NodeLabelsFetcherService will become factory or i will make 
it absolute.
        ConfigurationNodeLabelsProvider : will make it dynamic. i,e. 
periodically it will read the yarn-site and get the Labels.
 6) More implementation suggestions:
Since we need central node labels configuration, I suggest to leverage what we 
already have in RM admin CLI directly – user can use RM admin CLI add/remove 
node labels. We can disable this when we're ready to do non-central node label 
configuration.And there should be an option to tell if distributed node label 
configuration is used. If it's distributed, AdminService should disable admin 
change labels on nodes via RM admin CLI. I suggest to do this in a separated 
-- I presume "central node labels configuration" as "Cluster Valid Node Labels" 
stored at RM side for validation of labels if so ok will do it in the same way 
as that of RM Admin CLI
and for ??If it's distributed, AdminService should disable admin change labels 
on nodes via RM admin CLI?? will add a jira, 
but was wondering how to do this ? by configuration with new parameter? I was 
earlier under the impression as MemoryRMNodeLabelsManager => is for distributed 
Configuration and RMNodeLabelsManager is for Centrallized configuration. and 
some factory will take care of this....

Other comments will handle

> Allow admin specify labels in 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_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 or using script 
> suggested by [~aw])
> - 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