[
https://issues.apache.org/jira/browse/YARN-3075?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14292672#comment-14292672
]
Wangda Tan commented on YARN-3075:
----------------------------------
[~varun_saxena], Thanks for working on this, just reviewed.
*Comments:*
1) Following logic can be optimized:
{code}
case REPLACE:
Set<String> oldLabels = new HashSet<String>();
Set<String> hostLabels = getHostLabels(nodeId, nodeCollections);
if(hostLabels != null && !hostLabels.isEmpty()) {
oldLabels.addAll(hostLabels);
}
if(!nm.labels.isEmpty()) {
{code}
You can use {{getLabelsByNode}} to get labels from Host-Node hierarchy.
And oldLabels could be placed outside of the loop to avoid create the object
everytime.
2) getLabelsToNodes:
When there's no NodeLabel associated with label, it's better print warn message.
3) getHostLabels could be removed if 1) applied?
4) NodeLabel.getNodeIdInfo may not precise enough, rename to
getAssociatedNode(Id)s? (or other name if you have)
5) Also NodeLabel.getNodeIdInfo: I think we can assume NodeId will not changed
(nobody calls NodeId.setHost/Port), so copy reference should be enough, agree?
If yes, just return {{new HashSet<NodeId>(nodeIds)}}.
6) TestLabelsToNode:
{code}
// Replace labels on n1:1 to P2
mgr.replaceLabelsOnNode(ImmutableMap.of(toNodeId("n1:1"), toSet("p2"),
toNodeId("n1:2"), toSet("p2")));
labelsToNodes = mgr.getLabelsToNodes(null);
assertLabelsToNodesEquals(
labelsToNodes,
ImmutableMap.of(
"p1", toSet(toNodeId("n1")),
"p2", toSet(toNodeId("n1"),toNodeId("n1:1"),toNodeId("n1:2"))));
assertLabelsToNodesEquals(
labelsToNodes, transposeNodeToLabels(mgr.getNodeLabels()));
{code}
This seems not correct to me. n1 appears on p1 *and* p2, but label on host
should not include label on nodes of the host. Now I can understand why 1)
exists. I suggest to make this behavior consistent with getNodeLabels(), or at
least we can make getNodeLabels() consistent with getLabelsToNodes.
Thoughts?
Will include reviews for tests after we decided 6).
7) When a node/host has no label, it belongs to a special NodeLabel with key =
CommonNodeLabelsManager.NO_LABEL. This is necessary because node without label
can be considered as a "partition" as well. We need support it here (even if
getLabelsToNode not return it now.)
*Cosmetic suggestions:*
8) Add nodeId to Node to avoid loop like:
{code}
for (Entry<NodeId, Node> nmEntry : host.nms.entrySet()) {
Node node = nmEntry.getValue();
if (node.labels != null) {
node.labels.removeAll(labels);
}
removeNodeFromLabels(nmEntry.getKey(), labels);
}
{code}
> NodeLabelsManager implementation to retrieve label to node mapping
> ------------------------------------------------------------------
>
> Key: YARN-3075
> URL: https://issues.apache.org/jira/browse/YARN-3075
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: resourcemanager
> Affects Versions: 2.7.0
> Reporter: Varun Saxena
> Assignee: Varun Saxena
> Attachments: YARN-3075.001.patch, YARN-3075.002.patch
>
>
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
