aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Maxim Khutornenko" <ma...@apache.org>
Subject Re: Review Request 25812: Implementing quota checking for async job updates.
Date Tue, 23 Sep 2014 16:10:58 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message