[
https://issues.apache.org/jira/browse/YARN-2495?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14199494#comment-14199494
]
Wangda Tan commented on YARN-2495:
----------------------------------
*Major comments:*
1.
As I commented,
https://issues.apache.org/jira/browse/YARN-2495?focusedCommentId=14184146&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14184146,
first line.
I think the statement
bq. if number of labels are more than the weight of heartbeat message will be
more and if the cluster nodes are more than more network IO, checking of labels
from Prev state to current state for all nodes is done in RM in earlier method
each NM was taking care of it self. Lot of read locks needs to be held @
NodesLabelsManager
Makes sense to me, I suggest to add a field like node-labels-updated in
NodeHeartbeatRequest
*Minor comments:*
1.
bq. static NodeLabelsProviderService createNodeLabelsProviderService
It's better to make this not static or protected in case the tests can override
it. Since it is only used by test, I don't suggest to make it static for that.
2.
NodeStatusUpdater:
2.1
{code}
+ @SuppressWarnings("unchecked")
+ Set<String> nodeLabels =
+ (nodeLabelsProvider == null) ? Collections.EMPTY_SET
{code}
You can use CommonNodeLabelsManager##EMPTY_STRING_SET to avoid the warning
2.2
It's better to name areLabelsSameAsPrevOutPut -> areNodeLabelsUnchanged, it's
shorter and has same semantics of nodeLabelsUpdated.
2.3
Need handle null value for {{areNodeLabelsUnchanged}} (or
{{areLabelsSameAsPrevOutPut}} previously). Since others can implement a new
NodeLabelsProvider
2.4
bq. + Set<String> nodeLabelsForHeartBeat = null;
Shoud put in while (...) { }
3.
Suggest to create a overload newInstance function without labels for
RegisterNodeManagerRequest to avoid check like:
{code}
+ (nodeLabelsProvider == null) ? null : nodeLabels);
{code}
4.
YarnConfiguration:
- ENABLE_DECENTRALIZED_NODELABEL_CONFIGURATION ->
DECENTRALIZED_NODE_LABELS_CONFIGURATION_ENABLED
- labels-provider.class -> node-labels-provider.class
- For configuration file based options, I suggest to add a same prefix for
them, such as:
NM_NODE_LABELS_PREFIX + "config-based"
- NM_NODE_LABELS_FROM_CONFIG sounds like a boolean value to me, how about call
it NM_NODE_LABELS_PREFIX + "config-based" + ".node-labels"?
- NM_NODE_LABELS_FETCH_INTERVAL_MS should also be a part of config-based.
- Remove "+ /** End of Configurations in NodeManager for NodeLabelsFeature*/"
5. PB tests
I think you can leverage {{TestPBImplRecords}} do all PB related tests, does it
enough?
6. ConfigurationNodeLabelsProvider
- checkForNodeLabelModification: how about call it updateNodeLabelsFromConfig?
Since the checkForNodeLabelModification is more like a read-only operation to
me.
- Remove
{code}
+ } else {
+ if (LOG.isDebugEnabled())
+ LOG.debug(errorMsg.toString());
+ }
{code}
Since the error message is handled by ConfigurationMonitorTimerTask
- {{+ e.printStackTrace();}} to LOG.error(e)
7. NodeLabelsProviderService:
bq. @return
Need to be added for getNodeLabels, or javadocs warning will raise
8. Change the NodeHeartbeatResponse#getAcceptNodeLabels ->
getIsNodeLabelsAcceptedByRM, same as RegisterResponse (Or some beter name you
can think about, since the getAcceptedNodeLabels is more like a list of labels)
9. ResourceTrackerService:
9.1
{code}
+ LOG.info("Node Labels {" + StringUtils.join(",", nodeLabels)
+ + "} of NodeManager from " + host
+ + " is not properly configured: " + ex.getMessage() + " ");
{code}
Again, should use error here
9.2
The node label update logic for register/hearbeat are very similar, could you
merge common part of them?
Will include review of tests in next turn.
> 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_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)
