[
https://issues.apache.org/jira/browse/YARN-2495?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14371830#comment-14371830
]
Naganarasimha G R commented on YARN-2495:
-----------------------------------------
Thanks for review [~wangda] :
1) {{StringArrayProto.stringElement -> elements}}, while naming had done based
on Protoc but as you mentioned seems like
elements would make more sense in terms of java api, will correct it as part of
the next patch.
2) {quote} After thought, I think optional bool areNodeLabelsAcceptedByRM = 7
\[default = false\]; should be true to be more defensive: We need make sure
there's no error when somebody forget to set this field. {quote}
Well my view as explained earlier is lil diff, as even if some body forget it
test case will ensure they do not miss it but the case which i mentioned
earlier check whether its legitamate scenario
??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??
3) will be taken care in next patch
4) {quote}NodeLabelsProviderService -> NodeLabelsProvider, like most other
modules, we don't need to make "service" as a part of the classname, change sub
classes and NodeManager.createNodeLabelsProviderService as well.{quote}
Well i agree with this but long b4 you had only asked me to change it
NodeLabelsProviderService [Prev comment 4th
point|https://issues.apache.org/jira/browse/YARN-2495?focusedCommentId=14181031&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14181031].
will get it done in next patch
5) {quote}NodeStatusUpdaterImpl.run:
617 int lastHeartbeatID = 0;
618 Set<String> nodeLabelsLastUpdatedToRM = null;
619 if (hasNodeLabelsProvider)
No matter if hasNodeLabelsProvider, nodeLabelsLastUpdatedToRM should be null?
By default is "not change" instead of "empty", correct? {quote}
nodeLabelsLastUpdatedToRM => lastUpdatedNodeLabelsToRM i.e. its not the one
which will be sent as part of heartbeat (nodeLabelsForHeartbeat is used) and
nodeLabelsLastUpdatedToRM is reference for comparison whether labels have
changed since the last call. And as per the logic, NodeLabelsProvider even if
its return null we consider it as empty labels. So i feel its correctly set.
6) will take care in next patch
7) areNodeLabelsUpdated: Need check null? And could you add more test to cover
the case when new fetched node labels and/or last node labels are null?
{{Need check null}} has been taken care before calling this method, also you
had asked for commenting this which i have done in NodeLabelsProvider, wil do
this here too.
{{add more test to cover the case when new fetched node labels}} already there
in TestNodeStatusUpdaterForLabels.testNodeStatusUpdaterForNodeLabels(ln num 271
-277)
As explained in prev comment even though the NodeLabelsProvider return null
node labels we consider it as empty set hence there is no way last node labels
are null. Null is sent as part of heartbeat only when lastUpdatedNodeLabelsToRM
== nodeLabelsForHeartbeat . Will put some comments for this.
Please let me know your comments will try to give the patch ASAP.. :)
> 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.20150320-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
(v6.3.4#6332)
