> On Sept. 23, 2014, 1:01 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java, line 268
> > <https://reviews.apache.org/r/25812/diff/1/?file=694286#file694286line268>
> >
> >     Remember - javadoc is like html.  These newlines are not preserved 
> > unless you do it explicitly.  Drop a <p> here, ditto in other comments.

This was mostly for in-code readability but adding <p> would not hurt.


> On Sept. 23, 2014, 1:01 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java, line 274
> > <https://reviews.apache.org/r/25812/diff/1/?file=694286#file694286line274>
> >
> >     how about s/prodFromTasks/getRequiredQuota/?

Well, quota here means one thing - pre-defined cap to compare against. I feel 
it would be too confusing to overload that term for consumption. Changed it to 
prodResourcesFromTasks instead.


> On Sept. 23, 2014, 1:01 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
> >  line 354
> > <https://reviews.apache.org/r/25812/diff/1/?file=694289#file694289line354>
> >
> >     Catching IllegalArgumentException is likely to be more broad than you 
> > intend.  I suggest you throw a specific exception from the validate 
> > function to avoid incorrectly replying 400 for something that should be 500.

Agree, thanks for pushing it.


> On Sept. 23, 2014, 1:01 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
> >  line 1350
> > <https://reviews.apache.org/r/25812/diff/1/?file=694289#file694289line1350>
> >
> >     How about 'validateInstanceAddition'?  I'm not tied to the name, but 
> > something more informative than 'validate' would be nice.

How about validateTaskLimits?


> On Sept. 23, 2014, 1:01 a.m., Bill Farner wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 547
> > <https://reviews.apache.org/r/25812/diff/1/?file=694290#file694290line547>
> >
> >     Can you update JobUpdateControllerImpl to use this as well?
> >     
> >     Also, you should wrap this with a constant that we define.  Thrift 
> > produces a mutable static Set for this, which comes with risk.

The only place it's used in is already wrapping it into EnumSet. 

What specifically in JobUpdateControllerImpl? Could not find anything suitable 
to update.


- Maxim


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


On Sept. 23, 2014, 12:59 a.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25812/
> -----------------------------------------------------------
> 
> (Updated Sept. 23, 2014, 12:59 a.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 
> 779e925e4d9e7889e8cfd369cea9a8e5da3554d2 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  83ac034cff009530e5e16c0613b9d085f3b908d8 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> 2376a5e530b12fbbebb4cfc7555656ae07795518 
>   src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java 
> 49770e5f87f047502e4f5653b908657a40d8683f 
>   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
>  336cada625b85618486660fc24f3e83a312609f8 
>   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