Naganarasimha G R commented on YARN-2495:

1. Well i think i am missing something or you have miss-read my patch.
* in both request Protos i have used NodeIdToLabelsProto  {{optional 
NodeIdToLabelsProto nodeLabels = 8;}} and {{optional NodeIdToLabelsProto 
nodeLabels = 4;}} and not used {{repeat string nodeLabels}} directly
* Test cases TestYarnServerApiClasses.testNodeHeartbeatRequestPBImpl, 
testRegisterNodeManagerRequestWithNullLabels and 
testRegisterNodeManagerRequestWithValidLabels validates that the approach in 
the patch supports null and filled labelsset, manually tested for empty set and 
as expected that too worked. will get this test case added in next patch
* thought of creating a new proto like {{"StringSetProto"}} but felt why create 
another proto class just for this purpose and you too had mentioned to use 
{{NodeIdToLabelsProto}} hence made use of existing proto class 

2. {{Typo, lable -> label,}} : oops because of typo in proto, generated methods 
also had issues hence proto and places accessing these methods(6 instances) 
have this error. Will get it corrected in next patch.

3. ??optional bool areNodeLablesAcceptedByRM = 7 \[default = false\], I think 
default should be true.??
Personally i felt it should not matter as I am explicitly handling in the code. 
But consider the case where NM gets upgraded first then it should not be the 
case NM sends labels and older RM ignores the additional labels but response by 
default sends labels are accepted. And also felt by name/functionality, it 
should be set to true only after RM accepts the labels  

--will get it corrected as part of next patch.

Also one favor, it would be helpful if you review the test-cases and give feed 
back on them too, as it will reduce my effort in creating multiple patches. I 
understand that its huge size patch but anyway i feel major 
aspects/functionality seems to be stable with the last 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.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.20150318-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