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