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