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

Wangda Tan commented on YARN-2495:
----------------------------------

bq. I have already added the below code in the NodeHeartbeatRequest interface 
for this(or i dint get your comment correctly, please elaborate):
I think I've misreading your patch, your patch should have done what we 
discusse.

bq. This if check will be there in the overloaded case to identify which 
overloaded method to choose right ? i am not able to see any benefit from this.
The benefit are
1) You don't have to update test cases for that
2) The semanic are clear, create a register request with label or not.

bq. NM_NODE_LABELS_FETCH_INTERVAL_MS & DEFAULT_NM_NODE_LABELS_FETCH_INTERVAL_MS 
is used in both the script based and also config based hence had not moved it 
under config-based ? 
I suggest to have different option for script-based/config-based, even if we 
can combine them together.

bq. if reuse then can delete NM_NODE_LABELS_FROM_CONFIG, as its not used.
IIUC, NM_NODE_LABELS_FROM_CONFIG is a list of labels, even if we want to 
separate the two properties, we cannot remove NM_NODE_LABELS_FROM_CONFIG, 
correct? 
(But I still suggest you to change it to: NM_NODE_LABELS_PREFIX + 
"config-based" + ".node-labels".)

bq. These i wanted to discuss with you , based on your patch changes for labels 
had figured out this class but as i was modifying the existing PB class awas 
wondering why these existing PB's are not added here.
I think it's better to leverage existing utility class instead of implement 
your own. For example, you have set values but not check them, which is 
incorrect, but using utility class can avoid such problem. Even if you added 
new fields, tests will cover them without any changes:
{code}
-            ApplicationId.newInstance(1234L, 2)));
+            ApplicationId.newInstance(1234L, 2)),new HashSet<String>());
{code}

Will include review of unit tests after you uploaded new patch.

> 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_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
(v6.3.4#6332)

Reply via email to