[ 
https://issues.apache.org/jira/browse/YARN-3964?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14938278#comment-14938278
 ] 

Naganarasimha G R commented on YARN-3964:
-----------------------------------------

Thanks for the patch [~dian.fu],
bq. I think it should be fine to me if we solved the locking issue.
By this comment, IIUC [~wangda] meant about the issue which i mentioned. And as 
per the approach discussed it would be better to be handled in 
RMNodeLabelsManager (for which i am coming up with a jira and patch) so that it 
solves issue in all types of Node label configuration (centralized , 
distributed  & this). But still feel the approach in the new patch is better as 
it would be holding less locks on RMNodeLabelsManager and will  try to club and 
fetch multiple requests at one shot!
Few comments :
# Synchronizations in RMDelegatedNodeLabelsUpdaterTimerTask is not proper. {{ 
synchronized(this)}} holds lock on RMDelegatedNodeLabelsUpdaterTimerTask 
instance but newlyRegisteredNodes is updated in {{updateNodeLabels}} with the 
lock on RMDelegatedNodeLabelsUpdater instance.
# {{nodesToUpdateLabels == null}} is req in the below code ?
{code}
if (nodesToUpdateLabels == null && !newlyRegisteredNodes.isEmpty()) {
                synchronized(this) {
                  if (!newlyRegisteredNodes.isEmpty()) {
                    nodesToUpdateLabels = new 
HashSet<NodeId>(newlyRegisteredNodes);
                  }
                }
              }
{code} 
{{nodesToUpdateLabels == null}} is req ?
# 5 Seconds span i feel is little too much, better to have 30 seconds, if the 
provider is doing some operation which takes more than 5 seconds then multiple 
tasks can pile up.

Hope you can share your test code for the RMNodeLabelMappingsUpdater with which 
it i can test, hope you also have verified it .

> 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.007.patch, YARN-3964.007.patch, 
> YARN-3964.008.patch, YARN-3964.009.patch, YARN-3964.010.patch, 
> YARN-3964.011.patch, YARN-3964.012.patch, YARN-3964.013.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
(v6.3.4#6332)

Reply via email to