Wangda Tan commented on YARN-3964:


Some comments:
1) I suggest to make this to be an explicit node label configuration type: 
{{yarn.node-labels.configuration-type}}. Currently it has 
"centralized/distributed", I think you may add a "delegated-centralized" (or 
other better name).
Other configurations in your patch look fine to me.

2) Some comments of organization of Updater/Provider:
- Updater is a subclass of AbstractService, but no need to be abstract. I'm not 
sure what's the purpose of adding an AbstractNodeLabelsUpdater. Provider will 
be initialized by Updater, and Updater will call Provider's method periodically 
and notify RMNodeLabelsManager.
- Provider is an interface, minor comments to your patch:
** Why need a Configuration in getNodeLabels method?
** Returns Set<NodeLabel> instead of Set<String>

3) There're some methods / comments include "Fetcher", could you replace them 
to "Provider"?

4) Instead of adding a new checkAndThrowIfNodeLabelsFetcherConfigured, I 
suggest to reuse the checkAndThrowIfDistributedNodeLabelConfEnabled: You can 
rename it to something like checkAndThrowIfNodeLabelCannotBeUpdatedManually, 
which will check {{yarn.node-labels.configuration-type}}, we only allow 
manually update labels when type=centralized configured.

5) You can add a method to get RMNodeLabelsUpdater from RMContext, and remove 
it from ResourceTrackerService constructor.

6) Add a test of RMNodeLabelsUpdater? It seems can only update labels-on-node 
once for every node.

7) I think we need to make sure labels will be updated *synchronizedly* when a 
node is registering, this can avoid a node's labels initialized after it 
registered a while.

8) If you agree with #7, I think wait/notify implementation of Updater could be 
removed, you can use synchronized lock instead. Code using wait/notify has bad 
readability and will likely introduce bugs.


> Support NodeLabelsProvider at Resource Manager side
> ---------------------------------------------------
>                 Key: YARN-3964
>                 URL: https://issues.apache.org/jira/browse/YARN-3964
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>            Reporter: Dian Fu
>            Assignee: Dian Fu
>         Attachments: YARN-3964 design doc.pdf, YARN-3964.002.patch, 
> YARN-3964.003.patch, YARN-3964.004.patch, YARN-3964.005.patch, 
> YARN-3964.006.patch, YARN-3964.1.patch
> Currently, CLI/REST API is provided in Resource Manager to allow users to 
> specify labels for nodes. For labels which may change over time, users will 
> have to start a cron job to update the labels. This has the following 
> limitations:
> - The cron job needs to be run in the YARN admin user.
> - This makes it a little complicate to maintain as users will have to make 
> sure this service/daemon is alive.
> Adding a Node Labels Provider in Resource Manager will provide user more 
> flexibility.

This message was sent by Atlassian JIRA

Reply via email to