Mailing-List: contact yarn-issues-help@hadoop.apache.org; run by ezmlm
Precedence: bulk
Reply-To: yarn-issues@hadoop.apache.org
Date: Tue, 13 May 2014 07:28:15 +0000 (UTC)
From: "Wangda Tan (JIRA)" <jira@apache.org>
To: yarn-issues@hadoop.apache.org
Message-ID: <JIRA.12712070.1399073026290.336034.1399966095832@arcas>
In-Reply-To: <JIRA.12712070.1399073026290@arcas>
References: <JIRA.12712070.1399073026290@arcas>
Subject: [jira] [Commented] (YARN-2017) Merge some of the common lib code in
 schedulers
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


    [ https://issues.apache.org/jira/browse/YARN-2017?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13996165#comment-13996165 ] 

Wangda Tan commented on YARN-2017:
----------------------------------

Hi Jian,
Thanks for your efforts on this patch, some comments,

1) SchedulerNode.java
{code}
  private synchronized void deductAvailableResource(Resource resource) {
    if (resource == null) {
      LOG.error("Invalid deduction of null resource for "
          + rmNode.getNodeAddress());
{code}
Since this is original logic of SchedulerNode, I think it's better to throw exception instead of print a irresponsible log here. Null object passed in should be considered big problem in scheduler. And several following places in this class.

2) SchedulerNode.java
{code}
+  private synchronized boolean isValidContainer(Container c) {
+    if (launchedContainers.containsKey(c.getId()))
+      return true;
+    return false;
+  }
{code}
Better add "{..}" following "if"

3) SchedulerNode.java
{code}
+  public synchronized RMContainer getReservedContainer() {
+    return reservedContainer;
+  }
{code}
I think it's better to add a setReservedContainer(...) instead of manipulating super.reservedContainer in its sub classes. And change "protected reservedContainer" to private

4) In YarnScheduler.java
{code}
+  /**
+   * Get the whole resource capacity of the cluster.
+   * @return the whole resource capacity of the cluster.
+   */
+  @LimitedPrivate("yarn")
+  @Unstable
+  public Resource getClusterResource();
{code}
I'm wondering if it is meaningful to merge this method, too much code changes due to this merge. I found there're no common logic (like SchedulerNode/SchedulerAppAttempt) use it.

5) In FairScheduler.java
{code}
+  protected FSSchedulerApp getCurrentAttemptForContainer(ContainerId containerId) {
+    return (FSSchedulerApp) super.getCurrentAttemptForContainer(containerId);
}
{code}
I understand this is a "adaptor", I agree with [~sandyr] about using generic to eliminate such type casting?

> Merge some of the common lib code in schedulers
> -----------------------------------------------
>
>                 Key: YARN-2017
>                 URL: https://issues.apache.org/jira/browse/YARN-2017
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: resourcemanager
>            Reporter: Jian He
>            Assignee: Jian He
>         Attachments: YARN-2017.1.patch
>
>
> A bunch of same code is repeated among schedulers, e.g:  between FicaSchedulerNode and FSSchedulerNode. It's good to merge and share them in a common base.


--
This message was sent by Atlassian JIRA
(v6.2#6252)