hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Wangda Tan (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-2495) Allow admin specify labels in each NM (Distributed configuration)
Date Fri, 24 Oct 2014 17:53:35 GMT

    [ 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)

Mime
View raw message