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

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

Hi Naga,
Thanks for working on this patch, 
Comments round #1,

1) YarnConfiguration:
I think we should add a DEFAULT_DECENTRALIZED_NODELABEL_CONFIGURATION_ENABLED = 
false to avoid hardcode the "false" in implementations

2) NodeHeartbeatRequestPBImpl:
I just found current PB cannot tell difference between null and empty for 
repeated fields. And in your implementation, empty set will be always returned 
no matter the field is not being set or set an empty set.
So what we defined null for "not changed", empty for "no label" not establish 
any more.
What we can do is,
# Add a new field in NodeHeartbeatRequest, like "boolean nodeLabelUpdated".
# Use the add/removeLabelsOnNodes API provided by RMNodeLabelsManager, 
everytime pass the changed labels only.
# Everytime set the up-to-date labels to NodeHeartbeatRequest when do heartbeat.

#1 and #2 will all need add more fields in NodeHeartbeatRequest. I suggest to 
do in #3, it's more simple and we can improve it in further JIRA.

3) NodeManager:
{code}
+    if (conf.getBoolean(
+        YarnConfiguration.ENABLE_DECENTRALIZED_NODELABEL_CONFIGURATION, 
false)) {
{code}
Instead of hardcode "false" here, we should use 
"DEFAULT_DECENTRALIZED_NODELABEL_CONFIGURATION_ENABLED" instead.

bq. +      addService((Service) provider);
Why do this type conversion? I think we don't need it.

bq. createNodeStatusUpdater
I suggest to create a overload method without the nodeLabelsProviderService to 
avoid lots of changes in test/mock classes.

4) NodeLabelsProviderService:
It should extends AbstractService, there're some default implementations in 
AbstractService, we don't need implement all of them.

5) NodeStatusUpdaterImpl:
{{isDecentralizedNodeLabelsConf}} may not need here, if nodeLablesProvider 
passed in is null. That means {{isDecentralizedNodeLabelsConf}} is false. 

{code}
+            nodeLabelsForHeartBeat = null;
+            if (isDecentralizedNodeLabelsConf) {
...
{code}
According to my comment 2), I suggest to make it simple -- if provider is not 
null, set NodeHeartbeatRequest.nodeLabels to labels get from provider.

{code}
+            if (nodeLabelsForHeartBeat != null
+                && response.getDiagnosticsMessage() != null) {
+              LOG.info("Node Labels were rejected from RM "
+                  + response.getDiagnosticsMessage());
+            }
{code}
We cannot assume when diagosticMessage is not null, it is the node label 
rejected. I sugguest to add rejected-node-labels field to RegisterNMResponse 
and NodeHeartbeatResponse. Existing behavior in RMNodeLabelsManager is, if any 
of the labels is not valid, all labels will be rejected. What you should do is,
# In RM ResourceTracker, if exception raise when replace labels on node, put 
the new labels to reject node labels to response.
# In NM NodeStatusUpdater, if reject node labels is not null, LOG.error 
rejected node labels, and print diagnostic message.

As 3) suggested, create an overload constructor to avoid lots of changes in 
tests.

6) yarn_server_common_service_protos.proto
I think you miss adding nodeLabels to {{RegisterNodeManagerResponseProto}}, 
which should be in {{RegisterNodeManagerRequestProto}} ? :)

7) ConfigurationNodeLabelsProvider:
{code}
+    String[] nodeLabelsFromScript =
+        
StringUtils.getStrings(conf.get(YarnConfiguration.NM_NODE_LABELS_PREFIX, ""));
{code}
# nodeLabelsFromScript -> nodeLabelsFromConfiguration
# YarnConfiguration.NM_NODE_LABELS_PREFIX -> add an option like 
YarnConfiguration.NM_NODE_LABELS_FROM_CONFIG (NM_NODE_LABELS_PREFIX + 
"from-config") or some name you prefer -- At least it shouldn't be a prefix.

8) TestEventFlow:
Just pass a null for nodeLabelsProvider not works?

9) ResourceTrackerService:
{code}
+    isDecentralizedNodeLabelsConf = conf.getBoolean(
+        YarnConfiguration.ENABLE_DECENTRALIZED_NODELABEL_CONFIGURATION, false);
{code}
Avoid hardcode config default here as suggested above.

It no need to send shutdown message when any of the labels not accepted by 
RMNodeLabelsManager. Just add them to a reject node labels list, and add 
diagnostic message should be enough.

{code}
+            + ", assigned nodeId " + nodeId + ", node labels { "
+            + nodeLabels.toString()+" } ";
{code}
You should use StringUtils.join when you want to get a set of labels to String, 
set.toString() not defined

More comments will be added when you addressed above comments and added tests 
for them.

Thanks,
Wangda


> Allow admin specify labels in 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_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