Return-Path: X-Original-To: apmail-hadoop-yarn-issues-archive@minotaur.apache.org Delivered-To: apmail-hadoop-yarn-issues-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 3A2E617CA2 for ; Fri, 24 Oct 2014 17:53:36 +0000 (UTC) Received: (qmail 301 invoked by uid 500); 24 Oct 2014 17:53:36 -0000 Delivered-To: apmail-hadoop-yarn-issues-archive@hadoop.apache.org Received: (qmail 247 invoked by uid 500); 24 Oct 2014 17:53:36 -0000 Mailing-List: contact yarn-issues-help@hadoop.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: yarn-issues@hadoop.apache.org Delivered-To: mailing list yarn-issues@hadoop.apache.org Received: (qmail 232 invoked by uid 99); 24 Oct 2014 17:53:35 -0000 Received: from arcas.apache.org (HELO arcas.apache.org) (140.211.11.28) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 24 Oct 2014 17:53:35 +0000 Date: Fri, 24 Oct 2014 17:53:35 +0000 (UTC) From: "Wangda Tan (JIRA)" To: yarn-issues@hadoop.apache.org Message-ID: In-Reply-To: References: Subject: [jira] [Commented] (YARN-2495) Allow admin specify labels in each NM (Distributed configuration) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-JIRA-FingerPrint: 30527f35849b9dde25b450d4833f0394 [ https://issues.apache.org/jira/browse/YARN-2495?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14183135#comment-14183135 ] Wangda Tan commented on YARN-2495: ---------------------------------- Hi Naga, Thanks for working on this patch, Comments round #1, 1) YarnConfiguration: I think we should add a DEFAULT_DECENTRALIZED_NODELABEL_CONFIGURATION_ENABLED = false to avoid hardcode the "false" in implementations 2) NodeHeartbeatRequestPBImpl: I just found current PB cannot tell difference between null and empty for repeated fields. And in your implementation, empty set will be always returned no matter the field is not being set or set an empty set. So what we defined null for "not changed", empty for "no label" not establish any more. What we can do is, # Add a new field in NodeHeartbeatRequest, like "boolean nodeLabelUpdated". # Use the add/removeLabelsOnNodes API provided by RMNodeLabelsManager, everytime pass the changed labels only. # Everytime set the up-to-date labels to NodeHeartbeatRequest when do heartbeat. #1 and #2 will all need add more fields in NodeHeartbeatRequest. I suggest to do in #3, it's more simple and we can improve it in further JIRA. 3) NodeManager: {code} + if (conf.getBoolean( + YarnConfiguration.ENABLE_DECENTRALIZED_NODELABEL_CONFIGURATION, false)) { {code} Instead of hardcode "false" here, we should use "DEFAULT_DECENTRALIZED_NODELABEL_CONFIGURATION_ENABLED" instead. bq. + addService((Service) provider); Why do this type conversion? I think we don't need it. bq. createNodeStatusUpdater I suggest to create a overload method without the nodeLabelsProviderService to avoid lots of changes in test/mock classes. 4) NodeLabelsProviderService: It should extends AbstractService, there're some default implementations in AbstractService, we don't need implement all of them. 5) NodeStatusUpdaterImpl: {{isDecentralizedNodeLabelsConf}} may not need here, if nodeLablesProvider passed in is null. That means {{isDecentralizedNodeLabelsConf}} is false. {code} + nodeLabelsForHeartBeat = null; + if (isDecentralizedNodeLabelsConf) { ... {code} According to my comment 2), I suggest to make it simple -- if provider is not null, set NodeHeartbeatRequest.nodeLabels to labels get from provider. {code} + if (nodeLabelsForHeartBeat != null + && response.getDiagnosticsMessage() != null) { + LOG.info("Node Labels were rejected from RM " + + response.getDiagnosticsMessage()); + } {code} We cannot assume when diagosticMessage is not null, it is the node label rejected. I sugguest to add rejected-node-labels field to RegisterNMResponse and NodeHeartbeatResponse. Existing behavior in RMNodeLabelsManager is, if any of the labels is not valid, all labels will be rejected. What you should do is, # In RM ResourceTracker, if exception raise when replace labels on node, put the new labels to reject node labels to response. # In NM NodeStatusUpdater, if reject node labels is not null, LOG.error rejected node labels, and print diagnostic message. As 3) suggested, create an overload constructor to avoid lots of changes in tests. 6) yarn_server_common_service_protos.proto I think you miss adding nodeLabels to {{RegisterNodeManagerResponseProto}}, which should be in {{RegisterNodeManagerRequestProto}} ? :) 7) ConfigurationNodeLabelsProvider: {code} + String[] nodeLabelsFromScript = + StringUtils.getStrings(conf.get(YarnConfiguration.NM_NODE_LABELS_PREFIX, "")); {code} # nodeLabelsFromScript -> nodeLabelsFromConfiguration # YarnConfiguration.NM_NODE_LABELS_PREFIX -> add an option like YarnConfiguration.NM_NODE_LABELS_FROM_CONFIG (NM_NODE_LABELS_PREFIX + "from-config") or some name you prefer -- At least it shouldn't be a prefix. 8) TestEventFlow: Just pass a null for nodeLabelsProvider not works? 9) ResourceTrackerService: {code} + isDecentralizedNodeLabelsConf = conf.getBoolean( + YarnConfiguration.ENABLE_DECENTRALIZED_NODELABEL_CONFIGURATION, false); {code} Avoid hardcode config default here as suggested above. It no need to send shutdown message when any of the labels not accepted by RMNodeLabelsManager. Just add them to a reject node labels list, and add diagnostic message should be enough. {code} + + ", assigned nodeId " + nodeId + ", node labels { " + + nodeLabels.toString()+" } "; {code} You should use StringUtils.join when you want to get a set of labels to String, set.toString() not defined More comments will be added when you addressed above comments and added tests for them. Thanks, Wangda > Allow admin specify labels in 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_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)