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