[jira] [Commented] (YARN-1709) Admission Control: Reservation subsystem

["Chris Douglas (JIRA)" <ji...@apache.org>]

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

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

First pass:

{{RLE::addInterval/removeInterval}}
* This can return true when totCap == 0; when there's no work to do
* There are some redundant calls to {{isSameAsPrevious()}} and {{isSameAsNext()}} when adding a non-zero interval (it wouldn't be in the set if it were equal, so applying the same delta to each preserves this)
* Allowing a min/max allocation for the RLE would make this data structure more general/reusable outside this context
* These return true for all cases (the {{result}} variable is not necessary). This makes sense, until it adds invariants like min/max constraints.
* {{removeInterval}}: is it sufficient to compare against Resource(0, 0) instead of each member separately?
* {{removeInterval}}: doesn't roll back the transaction when it throws an exception, so it could leave an interval partially applied.

{{RLE::getCapacityAtTime/get*}}
* Many of the {{get*}} methods return mutable data, violating the locking. These should clone the objects before returning them.

{{RLE::toMemJSONString}}/{{RLE::toString}}
* Consider removing the spaces/newlines in the JSON representation
* Please use a {{StringBuilder}} instead of concatenation 
* Please use one of the JSON libraries on the classpath
* The {{toString()}} could be very verbose. Consider printing a summary (#steps, min/max, etc.) instead.

{{InMemoryPlan}}
* This should use a consistent clock time for the tick and archive, or it may archive ticks that have not been observed.
* The logic using {{isSuccess}} is confusing. Instead, return false/throw as constraints and invariants are violated, and return true when successful.
* {{ReentrantReadWriteLock}} is much slower w/ fair == true; are those semantics required?
* Using {{Class::isInstance}} is unconventional; using the instanceof operator or equals() (if it requires an exact match) is more common
* The {{*Cis}} fields and functions appear misnamed
* The {{updateCis}} function should be two functions, rather than passing {{addOrRemove}}
* {{updateCis}} would be easier to read if it established the invariant of the user in the collection, then called {{addInterval}} at the end.
* It looks like users in {{userCis}} are not GC'd
** If this is fixed, there's a potential NPE on {{userCis.get(reservation.getUser())}} deref
* {{getAllReservations}} should be package-private, {{@VisibleForTesting}} ; remove from interface
* {{headMap}} doesn't return null; these checks can be removed
* This returns mutable {{Resource}} instances from some {{get*}} methods, violating locking
* This creates many instances of {{Resource(0, 0)}}; can some of these be avoided?
* This should probably clone the {{Resource}} passed to setTotalCapacity
* Please remove the newlines in {{toString()}}
 
{{InMemoryReservationAllocation}}
* Fields can be final
* Why are some fields protected?
* remove newline from {{toString()}}
* Since this implements {{compareTo}}, it should also implement {{equals()}} and (particularly since it's added to collections calling it) {{hashCode()}}

General/nit
* some lines are more than 80 characters
* Javadocs contain empty lines
* Instead of two lookups on the HashSet {{containsKey()}}/{{get()}}, this can call {{get()}} once and check for {{null}}


> 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
>
>
> 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.2#6252)