Naganarasimha G R commented on YARN-2495:

Hi [~wangda], Thanks for reviewing and sorry for the delayed reply ...
Major comments:
Makes sense to me, I suggest to add a field like node-labels-updated in 

I have already added the below code in the NodeHeartbeatRequest interface for 
this(or i dint get your comment correctly, please elaborate):

@@ -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 
+  public abstract boolean isNodeLabelsUpdated();
+  public abstract void setNodeLabelsUpdated(boolean isNodeLabelsUpdated);

3.Suggest to create a overload newInstance function without labels for 
RegisterNodeManagerRequest to avoid check like:
(nodeLabelsProvider == null) ? null : nodeLabels);
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.

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.
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.

5. PB tests
I think you can leverage TestPBImplRecords do all PB related tests, does it 
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

Reply via email to