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/08/22 22:55:37 UTC
Review Request 24995: Adding support for a pre-update quota check.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24995/
-----------------------------------------------------------
Review request for Aurora, David McLaughlin and Kevin Sweeney.
Bugs: AURORA-649
https://issues.apache.org/jira/browse/AURORA-649
Repository: aurora
Description
-------
First step towards checking quota in startJobUpdate RPC. The final thrift wiring is to follow.
Diffs
-----
src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java 7a1a09b94cfe6d4b58ef4e6ab958b71c4be1e9bd
src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 24145f52a17ec9b7458aa5578a5b99ef5b1cddab
src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 3dcb1c3e3d0138634b3d077c845ecc0d61d7fc0f
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java f679f38f0fcff8abf2442cfcd885cd202ac55528
src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java a0ef57ec8af2fd6993eda338da14d1f4346ed3a4
src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java fa611a913bad40a8c0515c578b394c460340e574
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 997ade0be58723d1d91725061bb128ccf45e25b4
Diff: https://reviews.apache.org/r/24995/diff/
Testing
-------
gradle -Pq build
Thanks,
Maxim Khutornenko
Re: Review Request 24995: Adding support for a pre-update quota check.
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24995/#review51334
-----------------------------------------------------------
Ship it!
lgtm.
- David McLaughlin
On Aug. 22, 2014, 8:55 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24995/
> -----------------------------------------------------------
>
> (Updated Aug. 22, 2014, 8:55 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Kevin Sweeney.
>
>
> Bugs: AURORA-649
> https://issues.apache.org/jira/browse/AURORA-649
>
>
> Repository: aurora
>
>
> Description
> -------
>
> First step towards checking quota in startJobUpdate RPC. The final thrift wiring is to follow.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java 7a1a09b94cfe6d4b58ef4e6ab958b71c4be1e9bd
> src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 24145f52a17ec9b7458aa5578a5b99ef5b1cddab
> src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 3dcb1c3e3d0138634b3d077c845ecc0d61d7fc0f
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java f679f38f0fcff8abf2442cfcd885cd202ac55528
> src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java a0ef57ec8af2fd6993eda338da14d1f4346ed3a4
> src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java fa611a913bad40a8c0515c578b394c460340e574
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 997ade0be58723d1d91725061bb128ccf45e25b4
>
> Diff: https://reviews.apache.org/r/24995/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24995: Adding support for a pre-update quota check.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24995/#review51781
-----------------------------------------------------------
Ping, Kevin.
- Maxim Khutornenko
On Aug. 22, 2014, 8:55 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24995/
> -----------------------------------------------------------
>
> (Updated Aug. 22, 2014, 8:55 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Kevin Sweeney.
>
>
> Bugs: AURORA-649
> https://issues.apache.org/jira/browse/AURORA-649
>
>
> Repository: aurora
>
>
> Description
> -------
>
> First step towards checking quota in startJobUpdate RPC. The final thrift wiring is to follow.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java 7a1a09b94cfe6d4b58ef4e6ab958b71c4be1e9bd
> src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 24145f52a17ec9b7458aa5578a5b99ef5b1cddab
> src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 3dcb1c3e3d0138634b3d077c845ecc0d61d7fc0f
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java f679f38f0fcff8abf2442cfcd885cd202ac55528
> src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java a0ef57ec8af2fd6993eda338da14d1f4346ed3a4
> src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java fa611a913bad40a8c0515c578b394c460340e574
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 997ade0be58723d1d91725061bb128ccf45e25b4
>
> Diff: https://reviews.apache.org/r/24995/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24995: Adding support for a pre-update quota check.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24995/#review51845
-----------------------------------------------------------
Ship it!
Ship It!
- Kevin Sweeney
On Aug. 28, 2014, 1:58 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24995/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2014, 1:58 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Kevin Sweeney.
>
>
> Bugs: AURORA-649
> https://issues.apache.org/jira/browse/AURORA-649
>
>
> Repository: aurora
>
>
> Description
> -------
>
> First step towards checking quota in startJobUpdate RPC. The final thrift wiring is to follow.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java 7a1a09b94cfe6d4b58ef4e6ab958b71c4be1e9bd
> src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 24145f52a17ec9b7458aa5578a5b99ef5b1cddab
> src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 3dcb1c3e3d0138634b3d077c845ecc0d61d7fc0f
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java f679f38f0fcff8abf2442cfcd885cd202ac55528
> src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java a0ef57ec8af2fd6993eda338da14d1f4346ed3a4
> src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java fa611a913bad40a8c0515c578b394c460340e574
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 997ade0be58723d1d91725061bb128ccf45e25b4
>
> Diff: https://reviews.apache.org/r/24995/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24995: Adding support for a pre-update quota check.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Aug. 28, 2014, 10:07 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java, line 60
> > <https://reviews.apache.org/r/24995/diff/2/?file=671531#file671531line60>
> >
> > Rather than the deduped map would it make sense to take
> >
> > toKill: List[TaskConfig] - tasks to be killed
> > toCreate: List[TaskConfig] - tasks to be created
> >
> > and return
> >
> > sum(toKill.resources if toKill.production) + freeResources > sum(toCreate.resources if toCreate.production)
> >
> > Also, is the caller required to be in a write transaction for this to make sense? How does this function if there's a partial update in progress?
I thought about that but decided in favor of the current approach as the least involved from the quota validation standpoint. There will be some task diffing on the caller anyway where deduping would be a logical process of accumulating unique task configs. Besides, passing a list of new TaskConfigs seems rather unnatural.
*Also, is the caller required to be in a write transaction for this to make sense?*
Correct. See SchedulerCoreImpl.validateTaskLimits() description for example.
*How does this function if there's a partial update in progress?*
This method will serve both use cases:
1. "Umbrella" quota check before the update starts with both "release" and "add" parts provided.
2. Single instance add (from StateManager.insertPendingTasks) with only "add" part provided.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24995/#review51837
-----------------------------------------------------------
On Aug. 28, 2014, 8:58 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24995/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2014, 8:58 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Kevin Sweeney.
>
>
> Bugs: AURORA-649
> https://issues.apache.org/jira/browse/AURORA-649
>
>
> Repository: aurora
>
>
> Description
> -------
>
> First step towards checking quota in startJobUpdate RPC. The final thrift wiring is to follow.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java 7a1a09b94cfe6d4b58ef4e6ab958b71c4be1e9bd
> src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 24145f52a17ec9b7458aa5578a5b99ef5b1cddab
> src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 3dcb1c3e3d0138634b3d077c845ecc0d61d7fc0f
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java f679f38f0fcff8abf2442cfcd885cd202ac55528
> src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java a0ef57ec8af2fd6993eda338da14d1f4346ed3a4
> src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java fa611a913bad40a8c0515c578b394c460340e574
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 997ade0be58723d1d91725061bb128ccf45e25b4
>
> Diff: https://reviews.apache.org/r/24995/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24995: Adding support for a pre-update quota check.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24995/#review51837
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java
<https://reviews.apache.org/r/24995/#comment90463>
Rather than the deduped map would it make sense to take
toKill: List[TaskConfig] - tasks to be killed
toCreate: List[TaskConfig] - tasks to be created
and return
sum(toKill.resources if toKill.production) + freeResources > sum(toCreate.resources if toCreate.production)
Also, is the caller required to be in a write transaction for this to make sense? How does this function if there's a partial update in progress?
- Kevin Sweeney
On Aug. 28, 2014, 1:58 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24995/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2014, 1:58 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Kevin Sweeney.
>
>
> Bugs: AURORA-649
> https://issues.apache.org/jira/browse/AURORA-649
>
>
> Repository: aurora
>
>
> Description
> -------
>
> First step towards checking quota in startJobUpdate RPC. The final thrift wiring is to follow.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java 7a1a09b94cfe6d4b58ef4e6ab958b71c4be1e9bd
> src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 24145f52a17ec9b7458aa5578a5b99ef5b1cddab
> src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 3dcb1c3e3d0138634b3d077c845ecc0d61d7fc0f
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java f679f38f0fcff8abf2442cfcd885cd202ac55528
> src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java a0ef57ec8af2fd6993eda338da14d1f4346ed3a4
> src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java fa611a913bad40a8c0515c578b394c460340e574
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 997ade0be58723d1d91725061bb128ccf45e25b4
>
> Diff: https://reviews.apache.org/r/24995/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24995: Adding support for a pre-update quota check.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24995/
-----------------------------------------------------------
(Updated Aug. 28, 2014, 8:58 p.m.)
Review request for Aurora, David McLaughlin and Kevin Sweeney.
Changes
-------
CR comments.
Bugs: AURORA-649
https://issues.apache.org/jira/browse/AURORA-649
Repository: aurora
Description
-------
First step towards checking quota in startJobUpdate RPC. The final thrift wiring is to follow.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java 7a1a09b94cfe6d4b58ef4e6ab958b71c4be1e9bd
src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 24145f52a17ec9b7458aa5578a5b99ef5b1cddab
src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 3dcb1c3e3d0138634b3d077c845ecc0d61d7fc0f
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java f679f38f0fcff8abf2442cfcd885cd202ac55528
src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java a0ef57ec8af2fd6993eda338da14d1f4346ed3a4
src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java fa611a913bad40a8c0515c578b394c460340e574
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 997ade0be58723d1d91725061bb128ccf45e25b4
Diff: https://reviews.apache.org/r/24995/diff/
Testing
-------
gradle -Pq build
Thanks,
Maxim Khutornenko
Re: Review Request 24995: Adding support for a pre-update quota check.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Aug. 28, 2014, 8:48 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java, line 150
> > <https://reviews.apache.org/r/24995/diff/1/?file=667710#file667710line150>
> >
> > What's going on here? A comment explaining this formula would be helpful.
Added some comments around to help.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24995/#review51824
-----------------------------------------------------------
On Aug. 22, 2014, 8:55 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24995/
> -----------------------------------------------------------
>
> (Updated Aug. 22, 2014, 8:55 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Kevin Sweeney.
>
>
> Bugs: AURORA-649
> https://issues.apache.org/jira/browse/AURORA-649
>
>
> Repository: aurora
>
>
> Description
> -------
>
> First step towards checking quota in startJobUpdate RPC. The final thrift wiring is to follow.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java 7a1a09b94cfe6d4b58ef4e6ab958b71c4be1e9bd
> src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 24145f52a17ec9b7458aa5578a5b99ef5b1cddab
> src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 3dcb1c3e3d0138634b3d077c845ecc0d61d7fc0f
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java f679f38f0fcff8abf2442cfcd885cd202ac55528
> src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java a0ef57ec8af2fd6993eda338da14d1f4346ed3a4
> src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java fa611a913bad40a8c0515c578b394c460340e574
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 997ade0be58723d1d91725061bb128ccf45e25b4
>
> Diff: https://reviews.apache.org/r/24995/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24995: Adding support for a pre-update quota check.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24995/#review51824
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java
<https://reviews.apache.org/r/24995/#comment90441>
What's going on here? A comment explaining this formula would be helpful.
- Kevin Sweeney
On Aug. 22, 2014, 1:55 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24995/
> -----------------------------------------------------------
>
> (Updated Aug. 22, 2014, 1:55 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Kevin Sweeney.
>
>
> Bugs: AURORA-649
> https://issues.apache.org/jira/browse/AURORA-649
>
>
> Repository: aurora
>
>
> Description
> -------
>
> First step towards checking quota in startJobUpdate RPC. The final thrift wiring is to follow.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java 7a1a09b94cfe6d4b58ef4e6ab958b71c4be1e9bd
> src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 24145f52a17ec9b7458aa5578a5b99ef5b1cddab
> src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 3dcb1c3e3d0138634b3d077c845ecc0d61d7fc0f
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java f679f38f0fcff8abf2442cfcd885cd202ac55528
> src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java a0ef57ec8af2fd6993eda338da14d1f4346ed3a4
> src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java fa611a913bad40a8c0515c578b394c460340e574
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 997ade0be58723d1d91725061bb128ccf45e25b4
>
> Diff: https://reviews.apache.org/r/24995/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>