-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25812/#review54316
-----------------------------------------------------------


LGTM with some additional test coverage for the new accounting.  I think that 
will help make the desired output of the algorithm easier to understand in the 
future.


src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java
<https://reviews.apache.org/r/25812/#comment94420>

    Accept JobUpdateStore here, that's all that is used.



src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java
<https://reviews.apache.org/r/25812/#comment94422>

    Can you leave a longer comment here?  I understand what is going on, but 
i'm not sure other readers will have the context to follow the algorithm.



src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java
<https://reviews.apache.org/r/25812/#comment94423>

    Can you extract this predicate to a factory function?  I think it might aid 
readability.



src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java
<https://reviews.apache.org/r/25812/#comment94424>

    space between * and <p>



src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java
<https://reviews.apache.org/r/25812/#comment94425>

    space between * and <p>



src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java
<https://reviews.apache.org/r/25812/#comment94428>

    It would be great if these test cases validated the actual computed 
consumption here.  Can you also call quotaManager.getQuotaInfo to validate the 
accounting?



src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java
<https://reviews.apache.org/r/25812/#comment94427>

    Can you add 2 cases:
    - update adds instances
    - update removes instances


- Bill Farner


On Sept. 23, 2014, 4:48 p.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25812/
> -----------------------------------------------------------
> 
> (Updated Sept. 23, 2014, 4:48 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Kevin Sweeney, and Bill Farner.
> 
> 
> Bugs: AURORA-686
>     https://issues.apache.org/jira/browse/AURORA-686
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Getting it right required a deep refactoring of the QuotaManager. The prod 
> consumption is now calculated from:
> - production tasks not participating in job updates;
> - unaffected production tasks from a job being updated (i.e. those that are 
> not covered by the update working set);
> - production tasks directly affected by the job update (max of old and new 
> resources used).
> 
> Also, moved all logic back to SchedulerThriftInterface as quota checks are 
> done only there now.
> 
> 
> Diffs
> -----
> 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  3661f8487985f631e3ea437fe6430e0296376a9e 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 
> 14b0dd8f86026840d0444c128f656a144eab017d 
>   src/main/java/org/apache/aurora/scheduler/state/StateModule.java 
> 54b90127551c69509dbd41ee95c384dbbf1a7ee4 
>   src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java 
> 70333737ee119ea083f0ac17a4205e7af9f4c753 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  ae0320b44b55a3630e255484ea7a881daab6a7bc 
>   src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
> 8f9e6d6fa00840d8ff94eb7cf1c4415eb5cc1269 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  348bdbfbb51800d31b67ea1b3f2632975e598306 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> 2376a5e530b12fbbebb4cfc7555656ae07795518 
>   src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java 
> 8381e2126c62c40d28a325c72686d01e82bb84cf 
>   src/test/java/org/apache/aurora/scheduler/state/TaskLimitValidatorTest.java 
> 8f18617b2052201f87bb1464314c2ee45b279276 
>   
> src/test/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java
>  5aebbfbc691dfac4a066cb1425d18d3fccc77090 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  21e8a8ba5c47f549133ea339899bf0e5097ccbb9 
>   src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java 
> 40156c211a346664c0d2f174235efb2049cf3bb9 
> 
> Diff: https://reviews.apache.org/r/25812/diff/
> 
> 
> Testing
> -------
> 
> gradle -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Reply via email to