hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Chris Douglas (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-1709) Admission Control: Reservation subsystem
Date Sat, 06 Sep 2014 00:30:29 GMT

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

Chris Douglas commented on YARN-1709:
-------------------------------------

Overall, the patch lgtm. Just a few minor tweaks, then I'm +1
* very minor: Javadoc could be compressed a bit (empty lines)

{{InMemoryPlan}}
* The {{ZERO_RESOURCE}} instance escapes via {{getConsumptionForUser}}
* Some lines are more than 80 characters
* The logging can use built-in substitution more efficiently. Instead of:
{code}
String errMsg =
    MessageFormat
        .format(
            "The specified Reservation with ID {0} does not exist in the plan",
            reservation.getReservationId());
LOG.error(errMsg);
{code}
Prefer:
{code}
LOG.error("The specified Reservation with ID {} does not exist in the plan",
    reservation.getReservationId());
{code}
Some of the code already uses this construction, but a few still use {{MessageFormat}}.
* This form is harder to read:
{code}
InMemoryReservationAllocation inMemReservation = null;
if (reservation instanceof InMemoryReservationAllocation) {
  inMemReservation = (InMemoryReservationAllocation) reservation;
} else {
  // [snip] log error
  throw new RuntimeException(errMsg);
}
{code}
than the if (error) { throw; } construction used the other checks. Is it an improvement over
{{ClassCastException}}?
* {{addReservation}} doesn't need to hold the write lock while it checks invariants on its
arguments
* The private methods that assume locks ({{incrementAllocation}}, {{decrementAllocation}},
{{removeReservation}}, etc.) are held should probably {{assert}} that precondition (e.g.,
{{RRWL::isWriteLockedByCurrentThread()}})
* {{getMinimumAllocation}} and {{getMaximumAllocation}} return mutable data that should probably
be cloned

{{InMemoryReservationAllocation}}
* minor style: redundant {{this}} in get methods
* {{toString}} should use {{StringBuilder}} instead of {{StringBuffer}}

{{PlanView}}
* Mismatched javadoc on {{getEarliestStartTime}}
* {{getLastEndTime}} specifies UTC. Is that enforced in the implementation?

{{ReservationInterval}}
* Can this be made immutable? It's a key in several maps

{{RLESparseResourceAllocation}}
* Though some methods in {{InMemoryPlan}}, the {{ZERO_RESOURCE}} internal variable can escape
via {{getCapacityAtTime}}.

> Admission Control: Reservation subsystem
> ----------------------------------------
>
>                 Key: YARN-1709
>                 URL: https://issues.apache.org/jira/browse/YARN-1709
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: resourcemanager
>            Reporter: Carlo Curino
>            Assignee: Subramaniam Krishnan
>         Attachments: YARN-1709.patch, YARN-1709.patch, YARN-1709.patch
>
>
> This JIRA is about the key data structure used to track resources over time to enable
YARN-1051. The Reservation subsystem is conceptually a "plan" of how the scheduler will allocate
resources over-time.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message