> On Jan. 13, 2014, 10:58 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaComparisonResult.java, 
> > line 62
> > <https://reviews.apache.org/r/16629/diff/4/?file=420261#file420261line62>
> >
> >     This makes the details field kind of lame.  How about Optional<String> 
> > to make it obvious that it can be empty?

Done.


> On Jan. 13, 2014, 10:58 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaComparisonResult.java, 
> > line 66
> > <https://reviews.apache.org/r/16629/diff/4/?file=420261#file420261line66>
> >
> >     checkNotNull, ditto below

Done.


> On Jan. 13, 2014, 10:58 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaComparisonResult.java, 
> > line 75
> > <https://reviews.apache.org/r/16629/diff/4/?file=420261#file420261line75>
> >
> >     Convention dictates that accessors start with 'get'.  Please change to 
> > getResult() and getDetails().

Done.


> On Jan. 13, 2014, 10:58 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java, line 2
> > <https://reviews.apache.org/r/16629/diff/4/?file=420263#file420263line2>
> >
> >     Happy new year!  If you're using a file template in intellij, you can 
> > parameterize the year.

Same to you! :) Done.


> On Jan. 13, 2014, 10:58 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java, line 60
> > <https://reviews.apache.org/r/16629/diff/4/?file=420264#file420264line60>
> >
> >     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/

Thanks, rephrased.


> On Jan. 13, 2014, 10:58 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java, line 89
> > <https://reviews.apache.org/r/16629/diff/4/?file=420264#file420264line89>
> >
> >     checkNotNull ownerRole

Both args already checked in the SchedulerThriftInterface.setQuota().


> On Jan. 13, 2014, 10:58 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java, line 90
> > <https://reviews.apache.org/r/16629/diff/4/?file=420264#file420264line90>
> >
> >     New behavior, but while you're in here you might as well check these 
> > fields more fully (i.e. positive)

I presume we should still allow zeros there to allow partial or total 
"quota-lock" feature (i.e. when one or more specs are set to zero to prevent 
any additions/mutations)? Added ">=0" validation.


> On Jan. 13, 2014, 10:58 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/Quotas.java, line 32
> > <https://reviews.apache.org/r/16629/diff/4/?file=420266#file420266line32>
> >
> >     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.

Reduced and moved :) I am hesitant to completely drop this class as 
Quotas.noQuota() would still read better than QuotaManager.noQuota().


> On Jan. 13, 2014, 10:58 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java, 
> > line 131
> > <https://reviews.apache.org/r/16629/diff/4/?file=420268#file420268line131>
> >
> >     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.

Done.


> On Jan. 13, 2014, 10:58 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java, 
> > line 144
> > <https://reviews.apache.org/r/16629/diff/4/?file=420268#file420268line144>
> >
> >     Can you leave a TODO for me to remove the JobManager abstraction, and 
> > directly invoke addInstances here for non-cron jobs?

Done.


> On Jan. 13, 2014, 10:58 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java, 
> > line 167
> > <https://reviews.apache.org/r/16629/diff/4/?file=420268#file420268line167>
> >
> >     s/count/instances/ to be consistent with elsewhere?

Done.


> On Jan. 13, 2014, 10:58 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
> >  line 623
> > <https://reviews.apache.org/r/16629/diff/4/?file=420271#file420271line623>
> >
> >     i find the flow easier to follow if it were right after saveQuota in 
> > the try{}.

Done.


> On Jan. 13, 2014, 10:58 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java,
> >  line 164
> > <https://reviews.apache.org/r/16629/diff/4/?file=420275#file420275line164>
> >
> >     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.

That would require bumping up the constructor access level that I would rather 
not.


- Maxim


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


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