Chris Douglas commented on YARN-1709:

First pass:

* 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.

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

* 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.

* 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 
* {{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 
* This should probably clone the {{Resource}} passed to setTotalCapacity
* Please remove the newlines in {{toString()}}
* 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()}}

* 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

Reply via email to