Chris Douglas commented on YARN-1711:

- The public classes should be annotated with the correct visibility and 
stability annotations (probably {{@Public}} and {{@Unstable}})

- Javadoc throws clause and some parameters not populated
- In particular, the {{excludeList}} parameter could use some unpacking.

- Just performing the cast, and throwing {{ClassCastException}} implicitly is 
equally clear
- nit: spacing/concat: {{plan.getTotalCapacity() + ")by " + "accepting 
reservation: "}}
- Just as an observation, no change requested: the assumption that the sharing 
policy holds the lock on the plan is probably OK, but since both are interfaces 
there may be a missing abstraction that associates compatible sets of 
interlocking components.
- I can't think of a more appropriate solution to handling aggregates of 
{{Resource}}. Anything more "correct" doesn't really justify the complexity, 
certainly not before we get some more experience with planning. Since enabling 
this is optional, enforcement with the {{IntegralResource}} is a pragmatic 

- s/MismatchingUserException/MismatchedUserException/
- Are the subclasses of {{PlanningException}} for a caller to distinguish the 
cause for the rejected request, so it can refine it? If that's the case, should 
they contain diagnostic information as a payload e.g., requested vs actual 
user? If the intent is to extract it, then some more easily parsed format for 
the message might be appropriate (e.g., JSON).

- The {{excludeList}} should probably be final, and cleared/populated with a 
clone of the set on calls to {{init()}}

- Missing javadoc for the new parameters.

- Consider using {{@Test(expected = SomeException.class)}} instead of 
{{Assert::fail()}} and try/catch for {{testSingleFail()}}
- Consider specifying the expected cause/subtype instead of generic 
- {{testMultiTenantFail}} only veries that a {{PlanningException}} is thrown, 
not that it fails as expected

- Most of the tests don't verify that the failure occurs when and how its 
parameters specify, but only check that a {{PlanningException}} is thrown.

> CapacityOverTimePolicy: a policy to enforce quotas over time for YARN-1709
> --------------------------------------------------------------------------
>                 Key: YARN-1711
>                 URL: https://issues.apache.org/jira/browse/YARN-1711
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>            Reporter: Carlo Curino
>            Assignee: Carlo Curino
>              Labels: reservations
>         Attachments: YARN-1711.1.patch, YARN-1711.2.patch, YARN-1711.3.patch, 
> YARN-1711.patch
> This JIRA tracks the development of a policy that enforces user quotas (a 
> time-extension of the notion of capacity) in the inventory subsystem 
> discussed in YARN-1709.

This message was sent by Atlassian JIRA

Reply via email to