hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Wangda Tan (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-2017) Merge some of the common lib code in schedulers
Date Tue, 13 May 2014 07:28:15 GMT

    [ 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)

Mime
View raw message