Craig Welch commented on YARN-2495:

Other comments on the patch as such, assuming we really do need this part of 
the change...

DECENTRALIZED_CONFIGURATION_ENABLED et all - I do see the basis for enabling 
and disabling the enforcement of a centralized list and discussion around that, 
but I don't see any reason to have conditional enablement of the node manager 
side of things as well as a provider specification and I think it just adds 
unnecessary complexity and possible surprise at configuration time - at the 
level of this configuration I think it should just be enabled (and I don't mean 
just by default, I mean if we add this, it should just be a way to manage node 
labels, not conditionally enabled or disabled, any more than the web service or 
cli are conditionally enabled or disabled, and so we don't have this parameter 
/ it's associated branching at all).

I think the default node labels provider service should definitely be a null 
provider that always returns an empty list and areLabelsUpdated false - this 
takes out the need to decide "which is default", a no-op one is, and it allows 
us to get rid of the extra enabled/disabled configuration above without adding 
a new configuration (the provider will be specified anyway if the feature is 
going to be used)

NodeHeartbeatRequest - 

isNodeLabelsUpdated - I would go with areNodeLablesSet (all "isNodeLabels" => 
"areNodeLabels" wherever it appears, actually)  - wrt "Set" vs "Updated" - this 
is primarily a workaround for the null/empty ambiguity and I think this name 
better reflects what is really going on (am I sending a value to act on or 
not), but I also think that this is a better contract, the receiver (rm) 
shouldn't really care about the logic the nm side is using to decide whether or 
not to set it's labels (freshness, "updatedness", whatever), so all that should 
be communicated in the api is whether or not the value is set, not whether it's 
an update/whether it's checking freshness, etc.  that's a nit, but I think it's 
a clearer name.

RegisterNodeLabelManagerResponse - get/set IsNodeLabelsAcceptedByRM - I would 
make it get/set AreNodeLablesAcceptedByRM (and on imples, etc, of course)

RegisterNodeManagerRequest - missing spaces in args (l 42)  also, assuming we 
drop the "distributed on/off" config as I'm suggesting, you'll need the 
"areNodeLablesSet" to be passed here as well.  (I also like this better because 
it harmonizes the api between registration and heartbeat, which is easier to 
understand b/c they are doing the same thing / should do it the same way).

> 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.20141119-1.patch, YARN-2495.20141126-1.patch, 
> YARN-2495.20141204-1.patch, YARN-2495.20141208-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 (YARN-2923) or 
> using script suggested by [~aw] (YARN-2729) )
> - 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