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

Reply via email to