You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Maxim Khutornenko <ma...@apache.org> on 2014/10/08 00:03:06 UTC
Review Request 26425: Fixing quota checking for updates.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26425/
-----------------------------------------------------------
Review request for Aurora, Kevin Sweeney and Bill Farner.
Bugs: AURORA-802
https://issues.apache.org/jira/browse/AURORA-802
Repository: aurora
Description
-------
Rolling up prod consumption does not work for quota checking during job update intitiation where per-instance filtering is required. Splitting the QuotaManager interface into 2 use cases:
- createJob/addInstances
- startJobUpdate
Reverting the IResourceAggregate abstraction in QuotaManager in favor of task/update objects to simplify handling.
Diffs
-----
src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 2442b06f318c8d0cefe60296950baa1b916b42f7
src/main/java/org/apache/aurora/scheduler/quota/QuotaUtil.java d197dd515eb646cb4babadf22e9cf6480a919060
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 5dcae4a6132026504cf02093ad4c68ab521e4ab8
src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java b58c8f363e6e7c72accaf590b2a7cb7bf24275ea
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 02cd8f712fff3d283abf8e3eb1b4dcab1e762ac2
Diff: https://reviews.apache.org/r/26425/diff/
Testing
-------
./gradlew -Pq build
Also, tested various update scenarios in vagrant.
Thanks,
Maxim Khutornenko
Re: Review Request 26425: Fixing quota checking for updates.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Oct. 8, 2014, 5:17 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java, line 90
> > <https://reviews.apache.org/r/26425/diff/1/?file=714851#file714851line90>
> >
> > These methods read strangely to me: "check the quota of this task config", and "check the quota of this update".
> >
> > How about `checkInstanceAddition`, and `checkJobUpdate`?
Sure. Done.
> On Oct. 8, 2014, 5:17 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java, line 148
> > <https://reviews.apache.org/r/26425/diff/1/?file=714851#file714851line148>
> >
> > checkArgument(instances >= 0)
> >
> > (technically we could assert > 0, but this function should not care if instances == 0)
Done.
> On Oct. 8, 2014, 5:17 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java, line 168
> > <https://reviews.apache.org/r/26425/diff/1/?file=714851#file714851line168>
> >
> > The method names in QuotaInfo make this pretty confusing. It's not obvious why `getQuota()` and `getProdConsumpgion()` are used the way they are. To me this suggests the method names should be re-evaluated to avoid misuse.
The getQuota() is exactly what it is. I am not sure I have a better alternative for getProdConsumption() without using something stupidly long. Technically, it is a prod consumption in pure (IJobUpdate absent) or in a adjusted form where a not-yet-saved update is added to the prod mix to simplify handling. I have added javadoc comments for the getQuotaInfo() to hopefully make it clear.
> On Oct. 8, 2014, 5:17 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java, line 209
> > <https://reviews.apache.org/r/26425/diff/1/?file=714851#file714851line209>
> >
> > I would find this easier if functions were composed rather than combined. e.g.:
> >
> > // Fetch the active production tasks in a role, group by job, compute resources.
> > Map<IJobKey, IResourceAggregate> getProdTaskResources(String role);
> >
> > // Call getProdTaskResources, combine values.
> > IResourceAggregate getProdConsumption(String role);
> >
> > // Call getProdTaskResources, calculate charge for updated job, combine values.
> > IResourceAggregate getProdConsumptionDuringUpdate(IJobUpdate update);
The above would not quite work as the updates must be checked against while the prod consumption is computed to yield the correct value. Hence the combination rather than composition.
> On Oct. 8, 2014, 5:17 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java, line 317
> > <https://reviews.apache.org/r/26425/diff/1/?file=714851#file714851line317>
> >
> > Avoid encoding type information in the method name. Also, observe Law of Demeter. How about:
> >
> > private static IResourceAggregate toProdResources(IJobUpdateInstructions instructions)
Type encoding (Job Update) here is unintentional. Changed the name anyway.
> On Oct. 8, 2014, 5:17 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java, line 320
> > <https://reviews.apache.org/r/26425/diff/1/?file=714851#file714851line320>
> >
> > This is well-suited for a javadoc.
lol, it was actually a javadoc in QuotaUtil. Fixed.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26425/#review55819
-----------------------------------------------------------
On Oct. 7, 2014, 10:03 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26425/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2014, 10:03 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Bugs: AURORA-802
> https://issues.apache.org/jira/browse/AURORA-802
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Rolling up prod consumption does not work for quota checking during job update intitiation where per-instance filtering is required. Splitting the QuotaManager interface into 2 use cases:
> - createJob/addInstances
> - startJobUpdate
>
> Reverting the IResourceAggregate abstraction in QuotaManager in favor of task/update objects to simplify handling.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 2442b06f318c8d0cefe60296950baa1b916b42f7
> src/main/java/org/apache/aurora/scheduler/quota/QuotaUtil.java d197dd515eb646cb4babadf22e9cf6480a919060
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 5dcae4a6132026504cf02093ad4c68ab521e4ab8
> src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java b58c8f363e6e7c72accaf590b2a7cb7bf24275ea
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 02cd8f712fff3d283abf8e3eb1b4dcab1e762ac2
>
> Diff: https://reviews.apache.org/r/26425/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> Also, tested various update scenarios in vagrant.
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 26425: Fixing quota checking for updates.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26425/#review55819
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java
<https://reviews.apache.org/r/26425/#comment96194>
These methods read strangely to me: "check the quota of this task config", and "check the quota of this update".
How about `checkInstanceAddition`, and `checkJobUpdate`?
src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java
<https://reviews.apache.org/r/26425/#comment96196>
checkArgument(instances >= 0)
(technically we could assert > 0, but this function should not care if instances == 0)
src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java
<https://reviews.apache.org/r/26425/#comment96197>
The method names in QuotaInfo make this pretty confusing. It's not obvious why `getQuota()` and `getProdConsumpgion()` are used the way they are. To me this suggests the method names should be re-evaluated to avoid misuse.
src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java
<https://reviews.apache.org/r/26425/#comment96199>
I would find this easier if functions were composed rather than combined. e.g.:
// Fetch the active production tasks in a role, group by job, compute resources.
Map<IJobKey, IResourceAggregate> getProdTaskResources(String role);
// Call getProdTaskResources, combine values.
IResourceAggregate getProdConsumption(String role);
// Call getProdTaskResources, calculate charge for updated job, combine values.
IResourceAggregate getProdConsumptionDuringUpdate(IJobUpdate update);
src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java
<https://reviews.apache.org/r/26425/#comment96198>
Avoid encoding type information in the method name. Also, observe Law of Demeter. How about:
private static IResourceAggregate toProdResources(IJobUpdateInstructions instructions)
src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java
<https://reviews.apache.org/r/26425/#comment96200>
This is well-suited for a javadoc.
- Bill Farner
On Oct. 7, 2014, 10:03 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26425/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2014, 10:03 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Bugs: AURORA-802
> https://issues.apache.org/jira/browse/AURORA-802
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Rolling up prod consumption does not work for quota checking during job update intitiation where per-instance filtering is required. Splitting the QuotaManager interface into 2 use cases:
> - createJob/addInstances
> - startJobUpdate
>
> Reverting the IResourceAggregate abstraction in QuotaManager in favor of task/update objects to simplify handling.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 2442b06f318c8d0cefe60296950baa1b916b42f7
> src/main/java/org/apache/aurora/scheduler/quota/QuotaUtil.java d197dd515eb646cb4babadf22e9cf6480a919060
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 5dcae4a6132026504cf02093ad4c68ab521e4ab8
> src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java b58c8f363e6e7c72accaf590b2a7cb7bf24275ea
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 02cd8f712fff3d283abf8e3eb1b4dcab1e762ac2
>
> Diff: https://reviews.apache.org/r/26425/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> Also, tested various update scenarios in vagrant.
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 26425: Fixing quota checking for updates.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26425/#review55991
-----------------------------------------------------------
Ship it!
Ship It!
- Bill Farner
On Oct. 8, 2014, 10:58 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26425/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2014, 10:58 p.m.)
>
>
> Review request for Aurora and Bill Farner.
>
>
> Bugs: AURORA-802
> https://issues.apache.org/jira/browse/AURORA-802
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Rolling up prod consumption does not work for quota checking during job update intitiation where per-instance filtering is required. Splitting the QuotaManager interface into 2 use cases:
> - createJob/addInstances
> - startJobUpdate
>
> Reverting the IResourceAggregate abstraction in QuotaManager in favor of task/update objects to simplify handling.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 2442b06f318c8d0cefe60296950baa1b916b42f7
> src/main/java/org/apache/aurora/scheduler/quota/QuotaUtil.java d197dd515eb646cb4babadf22e9cf6480a919060
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 5dcae4a6132026504cf02093ad4c68ab521e4ab8
> src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java b58c8f363e6e7c72accaf590b2a7cb7bf24275ea
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 02cd8f712fff3d283abf8e3eb1b4dcab1e762ac2
>
> Diff: https://reviews.apache.org/r/26425/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> Also, tested various update scenarios in vagrant.
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 26425: Fixing quota checking for updates.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26425/
-----------------------------------------------------------
(Updated Oct. 8, 2014, 10:58 p.m.)
Review request for Aurora and Bill Farner.
Changes
-------
CR comments.
-kevints per request.
Bugs: AURORA-802
https://issues.apache.org/jira/browse/AURORA-802
Repository: aurora
Description
-------
Rolling up prod consumption does not work for quota checking during job update intitiation where per-instance filtering is required. Splitting the QuotaManager interface into 2 use cases:
- createJob/addInstances
- startJobUpdate
Reverting the IResourceAggregate abstraction in QuotaManager in favor of task/update objects to simplify handling.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 2442b06f318c8d0cefe60296950baa1b916b42f7
src/main/java/org/apache/aurora/scheduler/quota/QuotaUtil.java d197dd515eb646cb4babadf22e9cf6480a919060
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 5dcae4a6132026504cf02093ad4c68ab521e4ab8
src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java b58c8f363e6e7c72accaf590b2a7cb7bf24275ea
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 02cd8f712fff3d283abf8e3eb1b4dcab1e762ac2
Diff: https://reviews.apache.org/r/26425/diff/
Testing
-------
./gradlew -Pq build
Also, tested various update scenarios in vagrant.
Thanks,
Maxim Khutornenko
Re: Review Request 26425: Fixing quota checking for updates.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26425/#review55865
-----------------------------------------------------------
I'm underwater on reviews and unable to review this promptly - please remove me from the People line.
- Kevin Sweeney
On Oct. 7, 2014, 3:03 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26425/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2014, 3:03 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Bugs: AURORA-802
> https://issues.apache.org/jira/browse/AURORA-802
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Rolling up prod consumption does not work for quota checking during job update intitiation where per-instance filtering is required. Splitting the QuotaManager interface into 2 use cases:
> - createJob/addInstances
> - startJobUpdate
>
> Reverting the IResourceAggregate abstraction in QuotaManager in favor of task/update objects to simplify handling.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 2442b06f318c8d0cefe60296950baa1b916b42f7
> src/main/java/org/apache/aurora/scheduler/quota/QuotaUtil.java d197dd515eb646cb4babadf22e9cf6480a919060
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 5dcae4a6132026504cf02093ad4c68ab521e4ab8
> src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java b58c8f363e6e7c72accaf590b2a7cb7bf24275ea
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 02cd8f712fff3d283abf8e3eb1b4dcab1e762ac2
>
> Diff: https://reviews.apache.org/r/26425/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> Also, tested various update scenarios in vagrant.
>
>
> Thanks,
>
> Maxim Khutornenko
>
>