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



src/main/java/org/apache/aurora/scheduler/quota/QuotaComparisonResult.java
<https://reviews.apache.org/r/16629/#comment60352>

    This makes the details field kind of lame.  How about Optional<String> to 
make it obvious that it can be empty?



src/main/java/org/apache/aurora/scheduler/quota/QuotaComparisonResult.java
<https://reviews.apache.org/r/16629/#comment60355>

    checkNotNull, ditto below



src/main/java/org/apache/aurora/scheduler/quota/QuotaComparisonResult.java
<https://reviews.apache.org/r/16629/#comment60353>

    Convention dictates that accessors start with 'get'.  Please change to 
getResult() and getDetails().



src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java
<https://reviews.apache.org/r/16629/#comment60354>

    Happy new year!  If you're using a file template in intellij, you can 
parameterize the year.



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

    The doc and signature aren't currently enough to explain what this method 
does.  i.e. from this context it's not obvious what a "task change" is.  Looks 
like the missing details are that the check relates to the owner prescribed 
within 'template', and that the check is to validate whether the role can add 
'instanceCount' 'template'-sized tasks.
    
    Also, s/instanceCount/instances/



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

    checkNotNull ownerRole



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

    New behavior, but while you're in here you might as well check these fields 
more fully (i.e. positive)



src/main/java/org/apache/aurora/scheduler/quota/Quotas.java
<https://reviews.apache.org/r/16629/#comment60358>

    It's good to see some of these methods disappear.  It would be great to see 
the visibility of more of these methods reduced, or moved if there's only one 
caller.  This would further establish that the meaning of quota is owned by 
QuotaManager.  Please take a quick pass to see if any of these can be removed, 
moved, or reduce visibility.



src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java
<https://reviews.apache.org/r/16629/#comment60359>

    This comment might lose context quickly after this review is closed.  
Consider instead commenting above validateTaskLimits, noting that it's 
performed inside the transaction to avoid a data race.



src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java
<https://reviews.apache.org/r/16629/#comment60361>

    Can you leave a TODO for me to remove the JobManager abstraction, and 
directly invoke addInstances here for non-cron jobs?



src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java
<https://reviews.apache.org/r/16629/#comment60360>

    s/count/instances/ to be consistent with elsewhere?



src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
<https://reviews.apache.org/r/16629/#comment60364>

    i find the flow easier to follow if it were right after saveQuota in the 
try{}.



src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java
<https://reviews.apache.org/r/16629/#comment60368>

    Isn't it easier to just construct a fake value here, and avoid the stub?  I 
would find that easier to follow, anyhow, given that the class is purely a 
container.


- Bill Farner


On Jan. 10, 2014, 9:23 p.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16629/
> -----------------------------------------------------------
> 
> (Updated Jan. 10, 2014, 9:23 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Part 2: Server side changes for the client quota check. 
> 
> Refactored quota manager:
> - Merged QuotaFilter with QuotaManager and dropped JobFilter implementation;
> - Simplified quota manager logic by splitting data retrieval and quota 
> checking steps;
> - Moved quota checks into write transaction to ensure consistency.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/http/SchedulerzRole.java 
> cef0ff28bb0c0e08c5efaa1ed326f66bc9ffa5d9 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaComparisonResult.java 
> 99d2e4c72621708c971d25ad4e6722e0870093af 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaFilter.java 
> 6ab79820a0634478c0525d7fdd5a4d002ef8ea08 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 
> 6b0645ba93e50b576f7e572d8dc06231636fade2 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaModule.java 
> 4a619492f6e9eb41e693353187fc3b1781bffc1f 
>   src/main/java/org/apache/aurora/scheduler/quota/Quotas.java 
> 24f209339f3a6f4659693986e220187bd34d2fb5 
>   src/main/java/org/apache/aurora/scheduler/state/JobFilter.java 
> 0d84c1e2eff781e7d0250967ae6b9f9473fde3dc 
>   src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 
> 1d450f2d2d8e747878b67bccbf3fd7d018a52d20 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 79f56052a25ba756208e747dc5d198f30f0c4900 
>   
> src/main/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java
>  8fb51d69be6d370f9f010c797b2c1205b38a04f5 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  c1a11bdb91c5e764864324d26248d1783af8048b 
>   
> src/test/java/org/apache/aurora/scheduler/quota/QuotaComparisonResultTest.java
>  23069b8d191f1675636bceb8c297ebcc0d88d8dc 
>   src/test/java/org/apache/aurora/scheduler/quota/QuotaFilterTest.java 
> b1d878ea91c02ba87059b05877208b702d3fbcae 
>   src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java 
> f971aa1882e5e9f4208d177566779f5dd12d70ce 
>   
> src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java
>  720d0c86d8b112bf92196cbb81ece44476534654 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  91c1c24448092e1b3454844ab8074ed030383594 
>   src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java 
> cce27a0e37452f370a3729b6b05bf0bea29f85f6 
> 
> Diff: https://reviews.apache.org/r/16629/diff/
> 
> 
> Testing
> -------
> 
> gradle build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Reply via email to