[
https://issues.apache.org/jira/browse/YARN-2495?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14204974#comment-14204974
]
Naganarasimha G R commented on YARN-2495:
-----------------------------------------
Hi [~wangda], Thanks for reviewing and sorry for the delayed reply ...
{quote}
Major comments:
1.
Makes sense to me, I suggest to add a field like node-labels-updated in
NodeHeartbeatRequest
{quote}
I have already added the below code in the NodeHeartbeatRequest interface for
this(or i dint get your comment correctly, please elaborate):
{quote}
@@ -26,7 +28,8 @@
public static NodeHeartbeatRequest newInstance(NodeStatus nodeStatus,
MasterKey lastKnownContainerTokenMasterKey,
- MasterKey lastKnownNMTokenMasterKey) {
+ MasterKey lastKnownNMTokenMasterKey, Set<String> nodeLabels,
+ boolean isNodeLabelsUpdated
@@ -45,4 +50,10 @@ public static NodeHeartbeatRequest newInstance(NodeStatus
nodeStatus,
+ public abstract boolean isNodeLabelsUpdated();
+ public abstract void setNodeLabelsUpdated(boolean isNodeLabelsUpdated);
{quote}
{quote}
3.Suggest to create a overload newInstance function without labels for
RegisterNodeManagerRequest to avoid check like:
(nodeLabelsProvider == null) ? null : nodeLabels);
{quote}
This if check will be there in the overloaded case to identify which overloaded
method to choose right ? i am not able to see any benefit from this.
{quote}
4.YarnConfiguration:
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.
{quote}
NM_NODE_LABELS_FETCH_INTERVAL_MS & DEFAULT_NM_NODE_LABELS_FETCH_INTERVAL_MS is
used in both the script based and also config based hence had not moved it
under config-based ?
whats your opinion having two properties separately for script and config based
is better? if reuse then can delete NM_NODE_LABELS_FROM_CONFIG, as its not used.
{quote}
5. PB tests
I think you can leverage TestPBImplRecords do all PB related tests, does it
enough?
{quote}
These i wanted to discuss with you , based on your patch changes for labels had
figured out this class but as i was modifying the existing PB class awas
wondering why these existing PB's are not added here.
Others comments have reworked, after clarifications of the above points will
upload the patch tomorrow...
> 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)
