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)

* 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:
String errMsg =
            "The specified Reservation with ID {0} does not exist in the plan",
LOG.error("The specified Reservation with ID {} does not exist in the plan",
Some of the code already uses this construction, but a few still use 
* This form is harder to read:
InMemoryReservationAllocation inMemReservation = null;
if (reservation instanceof InMemoryReservationAllocation) {
  inMemReservation = (InMemoryReservationAllocation) reservation;
} else {
  // [snip] log error
  throw new RuntimeException(errMsg);
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

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

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

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

* 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

Reply via email to