You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@aurora.apache.org by Maxim Khutornenko <ma...@apache.org> on 2014/01/04 01:03:30 UTC
Review Request 16629: Client quota check (server side)
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16629/
-----------------------------------------------------------
Review request for Aurora, Kevin Sweeney and Bill Farner.
Repository: aurora
Description
-------
Part 2: Server side changes for the client quota check.
Note: api.thrift changes are duplicated from part 1: https://reviews.apache.org/r/16444/ and will race to submission with it.
Diffs
-----
src/main/java/org/apache/aurora/scheduler/state/LockManager.java 5f34de1bd0f90269642ea8d451ce4cd6fe180c2d
src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java bf0a650ae55eaa66ae7ee2b8a201cb5bda38177f
src/main/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java 8fb51d69be6d370f9f010c797b2c1205b38a04f5
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java c1a11bdb91c5e764864324d26248d1783af8048b
src/main/thrift/org/apache/aurora/gen/api.thrift 480b8f472bcfbe547a91c41638250350a0110334
src/test/java/org/apache/aurora/scheduler/state/LockManagerImplTest.java c8ad55d8d48f7e96180846ab515dd4df3d8ed79e
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 91c1c24448092e1b3454844ab8074ed030383594
src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 62fc8045f6a5fda234df73452685bd04e3142aaf
Diff: https://reviews.apache.org/r/16629/diff/
Testing
-------
gradle build
Thanks,
Maxim Khutornenko
Re: Review Request 16629: Client quota check (server side)
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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
>
>
Re: Review Request 16629: Client quota check (server side)
Posted by Bill Farner <wf...@apache.org>.
> 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.
>
> Maxim Khutornenko wrote:
> That would require bumping up the constructor access level that I would rather not.
>
> Bill Farner wrote:
> The convention of increasing visibility and marking @VisibleForTesting is much more established than stubbing returns. Frankly, i'm not a fan of stubbing returns since it makes assumptions about internal class behavior (or lack thereof).
>
> Maxim Khutornenko wrote:
> Don't really like widening up access just for testing but I buy your argument. Changed.
For a different perspective — it's odd for a package to contain a public interface that can't be implemented outside the package due to the return type constructor visibility.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16629/#review31664
-----------------------------------------------------------
On Jan. 14, 2014, 11:46 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16629/
> -----------------------------------------------------------
>
> (Updated Jan. 14, 2014, 11:46 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 785efd09a65568b174b35376c550ba5290e5915a
> 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 8dec2831444a21125d250d98132aae1009b0e306
> 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 76caa6214dc53c79222bc4a1b9e5066b61839ec6
> 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 4eeed38244904b64352ecc6c31111b66b2d0ced9
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java b46f29ad6cd1dd69fe31e73f2b8000dbf88508ea
> 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
>
>
Re: Review Request 16629: Client quota check (server side)
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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.
>
> Maxim Khutornenko wrote:
> That would require bumping up the constructor access level that I would rather not.
>
> Bill Farner wrote:
> The convention of increasing visibility and marking @VisibleForTesting is much more established than stubbing returns. Frankly, i'm not a fan of stubbing returns since it makes assumptions about internal class behavior (or lack thereof).
Don't really like widening up access just for testing but I buy your argument. Changed.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16629/#review31664
-----------------------------------------------------------
On Jan. 14, 2014, 2:03 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16629/
> -----------------------------------------------------------
>
> (Updated Jan. 14, 2014, 2:03 a.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
>
>
Re: Review Request 16629: Client quota check (server side)
Posted by Bill Farner <wf...@apache.org>.
> 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)
>
> Maxim Khutornenko wrote:
> 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.
sgtm
> 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.
>
> Maxim Khutornenko wrote:
> That would require bumping up the constructor access level that I would rather not.
The convention of increasing visibility and marking @VisibleForTesting is much more established than stubbing returns. Frankly, i'm not a fan of stubbing returns since it makes assumptions about internal class behavior (or lack thereof).
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16629/#review31664
-----------------------------------------------------------
On Jan. 14, 2014, 2:03 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16629/
> -----------------------------------------------------------
>
> (Updated Jan. 14, 2014, 2:03 a.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
>
>
Re: Review Request 16629: Client quota check (server side)
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16629/#review31664
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/quota/QuotaComparisonResult.java
<https://reviews.apache.org/r/16629/#comment60352>
This makes the details field kind of lame. How about Optional<String> to make it obvious that it can be empty?
src/main/java/org/apache/aurora/scheduler/quota/QuotaComparisonResult.java
<https://reviews.apache.org/r/16629/#comment60355>
checkNotNull, ditto below
src/main/java/org/apache/aurora/scheduler/quota/QuotaComparisonResult.java
<https://reviews.apache.org/r/16629/#comment60353>
Convention dictates that accessors start with 'get'. Please change to getResult() and getDetails().
src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java
<https://reviews.apache.org/r/16629/#comment60354>
Happy new year! If you're using a file template in intellij, you can parameterize the year.
src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java
<https://reviews.apache.org/r/16629/#comment60356>
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/
src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java
<https://reviews.apache.org/r/16629/#comment60357>
checkNotNull ownerRole
src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java
<https://reviews.apache.org/r/16629/#comment60363>
New behavior, but while you're in here you might as well check these fields more fully (i.e. positive)
src/main/java/org/apache/aurora/scheduler/quota/Quotas.java
<https://reviews.apache.org/r/16629/#comment60358>
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.
src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java
<https://reviews.apache.org/r/16629/#comment60359>
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.
src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java
<https://reviews.apache.org/r/16629/#comment60361>
Can you leave a TODO for me to remove the JobManager abstraction, and directly invoke addInstances here for non-cron jobs?
src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java
<https://reviews.apache.org/r/16629/#comment60360>
s/count/instances/ to be consistent with elsewhere?
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
<https://reviews.apache.org/r/16629/#comment60364>
i find the flow easier to follow if it were right after saveQuota in the try{}.
src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java
<https://reviews.apache.org/r/16629/#comment60368>
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.
- Bill Farner
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
>
>
Re: Review Request 16629: Client quota check (server side)
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16629/#review31924
-----------------------------------------------------------
Thanks. Pushed.
- Maxim Khutornenko
On Jan. 15, 2014, 8:56 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16629/
> -----------------------------------------------------------
>
> (Updated Jan. 15, 2014, 8:56 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 785efd09a65568b174b35376c550ba5290e5915a
> 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 8dec2831444a21125d250d98132aae1009b0e306
> 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 76caa6214dc53c79222bc4a1b9e5066b61839ec6
> 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 4eeed38244904b64352ecc6c31111b66b2d0ced9
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java b46f29ad6cd1dd69fe31e73f2b8000dbf88508ea
> 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
>
>
Re: Review Request 16629: Client quota check (server side)
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16629/
-----------------------------------------------------------
(Updated Jan. 15, 2014, 8:56 p.m.)
Review request for Aurora, Kevin Sweeney and Bill Farner.
Changes
-------
CR comments addressed.
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 (updated)
-----
src/main/java/org/apache/aurora/scheduler/http/SchedulerzRole.java 785efd09a65568b174b35376c550ba5290e5915a
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 8dec2831444a21125d250d98132aae1009b0e306
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 76caa6214dc53c79222bc4a1b9e5066b61839ec6
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 4eeed38244904b64352ecc6c31111b66b2d0ced9
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java b46f29ad6cd1dd69fe31e73f2b8000dbf88508ea
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
Re: Review Request 16629: Client quota check (server side)
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Jan. 15, 2014, 8:06 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java, line 148
> > <https://reviews.apache.org/r/16629/diff/5-6/?file=422351#file422351line148>
> >
> > static final? ditto below
Done.
> On Jan. 15, 2014, 8:06 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java, line 198
> > <https://reviews.apache.org/r/16629/diff/5-6/?file=422351#file422351line198>
> >
> > i'm this close to just letting intellij win the battle, but for now — revert.
This is crazy. I have spotted and fixed this issue in the original diff but it got reverted by IntelliJ in the latest round. It's present in diffs 1-5, gets reverted back to master version in 5-6 and now will have to be fixed again. So, yeah, I am totally in for giving up that battle :)
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16629/#review31911
-----------------------------------------------------------
On Jan. 14, 2014, 11:46 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16629/
> -----------------------------------------------------------
>
> (Updated Jan. 14, 2014, 11:46 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 785efd09a65568b174b35376c550ba5290e5915a
> 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 8dec2831444a21125d250d98132aae1009b0e306
> 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 76caa6214dc53c79222bc4a1b9e5066b61839ec6
> 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 4eeed38244904b64352ecc6c31111b66b2d0ced9
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java b46f29ad6cd1dd69fe31e73f2b8000dbf88508ea
> 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
>
>
Re: Review Request 16629: Client quota check (server side)
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16629/#review31911
-----------------------------------------------------------
Ship it!
src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java
<https://reviews.apache.org/r/16629/#comment60666>
static final? ditto below
src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java
<https://reviews.apache.org/r/16629/#comment60667>
i'm this close to just letting intellij win the battle, but for now — revert.
- Bill Farner
On Jan. 14, 2014, 11:46 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16629/
> -----------------------------------------------------------
>
> (Updated Jan. 14, 2014, 11:46 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 785efd09a65568b174b35376c550ba5290e5915a
> 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 8dec2831444a21125d250d98132aae1009b0e306
> 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 76caa6214dc53c79222bc4a1b9e5066b61839ec6
> 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 4eeed38244904b64352ecc6c31111b66b2d0ced9
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java b46f29ad6cd1dd69fe31e73f2b8000dbf88508ea
> 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
>
>
Re: Review Request 16629: Client quota check (server side)
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16629/
-----------------------------------------------------------
(Updated Jan. 14, 2014, 11:46 p.m.)
Review request for Aurora, Kevin Sweeney and Bill Farner.
Changes
-------
CR comments addressed.
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 (updated)
-----
src/main/java/org/apache/aurora/scheduler/http/SchedulerzRole.java 785efd09a65568b174b35376c550ba5290e5915a
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 8dec2831444a21125d250d98132aae1009b0e306
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 76caa6214dc53c79222bc4a1b9e5066b61839ec6
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 4eeed38244904b64352ecc6c31111b66b2d0ced9
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java b46f29ad6cd1dd69fe31e73f2b8000dbf88508ea
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
Re: Review Request 16629: Client quota check (server side)
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16629/
-----------------------------------------------------------
(Updated Jan. 14, 2014, 2:03 a.m.)
Review request for Aurora, Kevin Sweeney and Bill Farner.
Changes
-------
CR comments addressed.
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 (updated)
-----
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
Re: Review Request 16629: Client quota check (server side)
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
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.
Changes
-------
Added missing comments.
Repository: aurora
Description (updated)
-------
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 (updated)
-----
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
Re: Review Request 16629: Client quota check (server side)
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16629/
-----------------------------------------------------------
(Updated Jan. 10, 2014, 7:39 p.m.)
Review request for Aurora, Kevin Sweeney and Bill Farner.
Changes
-------
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.
Repository: aurora
Description (updated)
-------
Part 2: Server side changes for the client quota check.
Diffs (updated)
-----
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
Re: Review Request 16629: Client quota check (server side)
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Jan. 6, 2014, 9:30 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 597
> > <https://reviews.apache.org/r/16629/diff/2/?file=415181#file415181line597>
> >
> > We now have (at least) two places that get the current quota usage for a role. Worse yet, i argue that both of these are flawed since they don't account for cron jobs (the policy on that probably warrants wider discussion). For the time being, i suggest you extract a helper function to share between this and QuotaFilter.
My intention is to completely deprecate QuotaFilter and related. Here is the original plan (sorry for not communicating it earlier):
1. Enable quota client checks for job update (client side);
2. Expose consumed quota info to the client (server side);
3. Enable quota client checks for job create (client side);
4. Deploy all changes in prod;
5. Deprecate (remove) everything but Quotas.java under org.apache.aurora.scheduler.quota;
This CR is part 2 from the above plan. Step 3 will also cover cron job quota checks at creation time. I agree, we would need more discussion around cron job quota enforcement at task launch (if we ever decide to do so). Does it make sense?
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16629/#review31265
-----------------------------------------------------------
On Jan. 4, 2014, 1:53 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16629/
> -----------------------------------------------------------
>
> (Updated Jan. 4, 2014, 1:53 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Part 2: Server side changes for the client quota check.
>
> Note: api.thrift changes are duplicated from part 1: https://reviews.apache.org/r/16444/ and will race to submission with it.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java 8fb51d69be6d370f9f010c797b2c1205b38a04f5
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java c1a11bdb91c5e764864324d26248d1783af8048b
> src/main/thrift/org/apache/aurora/gen/api.thrift 480b8f472bcfbe547a91c41638250350a0110334
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 91c1c24448092e1b3454844ab8074ed030383594
>
> Diff: https://reviews.apache.org/r/16629/diff/
>
>
> Testing
> -------
>
> gradle build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 16629: Client quota check (server side)
Posted by Bill Farner <wf...@apache.org>.
> On Jan. 6, 2014, 9:30 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 597
> > <https://reviews.apache.org/r/16629/diff/2/?file=415181#file415181line597>
> >
> > We now have (at least) two places that get the current quota usage for a role. Worse yet, i argue that both of these are flawed since they don't account for cron jobs (the policy on that probably warrants wider discussion). For the time being, i suggest you extract a helper function to share between this and QuotaFilter.
>
> Maxim Khutornenko wrote:
> My intention is to completely deprecate QuotaFilter and related. Here is the original plan (sorry for not communicating it earlier):
> 1. Enable quota client checks for job update (client side);
> 2. Expose consumed quota info to the client (server side);
> 3. Enable quota client checks for job create (client side);
> 4. Deploy all changes in prod;
> 5. Deprecate (remove) everything but Quotas.java under org.apache.aurora.scheduler.quota;
>
> This CR is part 2 from the above plan. Step 3 will also cover cron job quota checks at creation time. I agree, we would need more discussion around cron job quota enforcement at task launch (if we ever decide to do so). Does it make sense?
>
I don't see how you could remove (the functionality of) QuotaFilter. How do you intend to enforce quotas otherwise?
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16629/#review31265
-----------------------------------------------------------
On Jan. 4, 2014, 1:53 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16629/
> -----------------------------------------------------------
>
> (Updated Jan. 4, 2014, 1:53 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Part 2: Server side changes for the client quota check.
>
> Note: api.thrift changes are duplicated from part 1: https://reviews.apache.org/r/16444/ and will race to submission with it.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java 8fb51d69be6d370f9f010c797b2c1205b38a04f5
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java c1a11bdb91c5e764864324d26248d1783af8048b
> src/main/thrift/org/apache/aurora/gen/api.thrift 480b8f472bcfbe547a91c41638250350a0110334
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 91c1c24448092e1b3454844ab8074ed030383594
>
> Diff: https://reviews.apache.org/r/16629/diff/
>
>
> Testing
> -------
>
> gradle build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 16629: Client quota check (server side)
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16629/#review31265
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
<https://reviews.apache.org/r/16629/#comment59655>
We now have (at least) two places that get the current quota usage for a role. Worse yet, i argue that both of these are flawed since they don't account for cron jobs (the policy on that probably warrants wider discussion). For the time being, i suggest you extract a helper function to share between this and QuotaFilter.
- Bill Farner
On Jan. 4, 2014, 1:53 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16629/
> -----------------------------------------------------------
>
> (Updated Jan. 4, 2014, 1:53 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Part 2: Server side changes for the client quota check.
>
> Note: api.thrift changes are duplicated from part 1: https://reviews.apache.org/r/16444/ and will race to submission with it.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java 8fb51d69be6d370f9f010c797b2c1205b38a04f5
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java c1a11bdb91c5e764864324d26248d1783af8048b
> src/main/thrift/org/apache/aurora/gen/api.thrift 480b8f472bcfbe547a91c41638250350a0110334
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 91c1c24448092e1b3454844ab8074ed030383594
>
> Diff: https://reviews.apache.org/r/16629/diff/
>
>
> Testing
> -------
>
> gradle build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 16629: Client quota check (server side)
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16629/
-----------------------------------------------------------
(Updated Jan. 4, 2014, 1:53 a.m.)
Review request for Aurora, Kevin Sweeney and Bill Farner.
Changes
-------
Dropped Lock from getQuota().
Repository: aurora
Description
-------
Part 2: Server side changes for the client quota check.
Note: api.thrift changes are duplicated from part 1: https://reviews.apache.org/r/16444/ and will race to submission with it.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java 8fb51d69be6d370f9f010c797b2c1205b38a04f5
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java c1a11bdb91c5e764864324d26248d1783af8048b
src/main/thrift/org/apache/aurora/gen/api.thrift 480b8f472bcfbe547a91c41638250350a0110334
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 91c1c24448092e1b3454844ab8074ed030383594
Diff: https://reviews.apache.org/r/16629/diff/
Testing
-------
gradle build
Thanks,
Maxim Khutornenko
Re: Review Request 16629: Client quota check (server side)
Posted by Kevin Sweeney <ke...@apache.org>.
> On Jan. 3, 2014, 4:55 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/state/LockManager.java, line 65
> > <https://reviews.apache.org/r/16629/diff/1/?file=415149#file415149line65>
> >
> > Is there a reason to take an Optional here? Shouldn't the caller just not call in if the provided lock is null?
>
> Maxim Khutornenko wrote:
> This is a convenience method for cases like getQuota() where Lock presence is optional as it's used in both locked (update, create) and unlocked (getQuota api) contexts. Having Optional here avoids IF condition on the SchedulerThriftInterface side.
I personally prefer that input sanitization happen closer to the request processing code when possible (which allows us to assume that no nulls are passed to core objects) but I'll defer to consensus here.
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16629/#review31197
-----------------------------------------------------------
On Jan. 3, 2014, 4:03 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16629/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2014, 4:03 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Part 2: Server side changes for the client quota check.
>
> Note: api.thrift changes are duplicated from part 1: https://reviews.apache.org/r/16444/ and will race to submission with it.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/state/LockManager.java 5f34de1bd0f90269642ea8d451ce4cd6fe180c2d
> src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java bf0a650ae55eaa66ae7ee2b8a201cb5bda38177f
> src/main/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java 8fb51d69be6d370f9f010c797b2c1205b38a04f5
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java c1a11bdb91c5e764864324d26248d1783af8048b
> src/main/thrift/org/apache/aurora/gen/api.thrift 480b8f472bcfbe547a91c41638250350a0110334
> src/test/java/org/apache/aurora/scheduler/state/LockManagerImplTest.java c8ad55d8d48f7e96180846ab515dd4df3d8ed79e
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 91c1c24448092e1b3454844ab8074ed030383594
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 62fc8045f6a5fda234df73452685bd04e3142aaf
>
> Diff: https://reviews.apache.org/r/16629/diff/
>
>
> Testing
> -------
>
> gradle build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 16629: Client quota check (server side)
Posted by Bill Farner <wf...@apache.org>.
> On Jan. 4, 2014, 12:55 a.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/state/LockManager.java, line 65
> > <https://reviews.apache.org/r/16629/diff/1/?file=415149#file415149line65>
> >
> > Is there a reason to take an Optional here? Shouldn't the caller just not call in if the provided lock is null?
>
> Maxim Khutornenko wrote:
> This is a convenience method for cases like getQuota() where Lock presence is optional as it's used in both locked (update, create) and unlocked (getQuota api) contexts. Having Optional here avoids IF condition on the SchedulerThriftInterface side.
>
> Kevin Sweeney wrote:
> I personally prefer that input sanitization happen closer to the request processing code when possible (which allows us to assume that no nulls are passed to core objects) but I'll defer to consensus here.
>
> Maxim Khutornenko wrote:
> Actually, it might even make sense to drop the Lock from getQuota() call altogether. The getQuota() does not make any changes to the system and both update and create flows would already have locks acquired by the time this RPC is called. So, validating lock context within getQuota() would not add anything. Any objections?
I agree with your thought — the lock on getQuota doesn't add much currently.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16629/#review31197
-----------------------------------------------------------
On Jan. 4, 2014, 12:03 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16629/
> -----------------------------------------------------------
>
> (Updated Jan. 4, 2014, 12:03 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Part 2: Server side changes for the client quota check.
>
> Note: api.thrift changes are duplicated from part 1: https://reviews.apache.org/r/16444/ and will race to submission with it.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/state/LockManager.java 5f34de1bd0f90269642ea8d451ce4cd6fe180c2d
> src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java bf0a650ae55eaa66ae7ee2b8a201cb5bda38177f
> src/main/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java 8fb51d69be6d370f9f010c797b2c1205b38a04f5
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java c1a11bdb91c5e764864324d26248d1783af8048b
> src/main/thrift/org/apache/aurora/gen/api.thrift 480b8f472bcfbe547a91c41638250350a0110334
> src/test/java/org/apache/aurora/scheduler/state/LockManagerImplTest.java c8ad55d8d48f7e96180846ab515dd4df3d8ed79e
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 91c1c24448092e1b3454844ab8074ed030383594
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 62fc8045f6a5fda234df73452685bd04e3142aaf
>
> Diff: https://reviews.apache.org/r/16629/diff/
>
>
> Testing
> -------
>
> gradle build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 16629: Client quota check (server side)
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Jan. 4, 2014, 12:55 a.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/state/LockManager.java, line 65
> > <https://reviews.apache.org/r/16629/diff/1/?file=415149#file415149line65>
> >
> > Is there a reason to take an Optional here? Shouldn't the caller just not call in if the provided lock is null?
>
> Maxim Khutornenko wrote:
> This is a convenience method for cases like getQuota() where Lock presence is optional as it's used in both locked (update, create) and unlocked (getQuota api) contexts. Having Optional here avoids IF condition on the SchedulerThriftInterface side.
>
> Kevin Sweeney wrote:
> I personally prefer that input sanitization happen closer to the request processing code when possible (which allows us to assume that no nulls are passed to core objects) but I'll defer to consensus here.
Actually, it might even make sense to drop the Lock from getQuota() call altogether. The getQuota() does not make any changes to the system and both update and create flows would already have locks acquired by the time this RPC is called. So, validating lock context within getQuota() would not add anything. Any objections?
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16629/#review31197
-----------------------------------------------------------
On Jan. 4, 2014, 12:03 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16629/
> -----------------------------------------------------------
>
> (Updated Jan. 4, 2014, 12:03 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Part 2: Server side changes for the client quota check.
>
> Note: api.thrift changes are duplicated from part 1: https://reviews.apache.org/r/16444/ and will race to submission with it.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/state/LockManager.java 5f34de1bd0f90269642ea8d451ce4cd6fe180c2d
> src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java bf0a650ae55eaa66ae7ee2b8a201cb5bda38177f
> src/main/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java 8fb51d69be6d370f9f010c797b2c1205b38a04f5
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java c1a11bdb91c5e764864324d26248d1783af8048b
> src/main/thrift/org/apache/aurora/gen/api.thrift 480b8f472bcfbe547a91c41638250350a0110334
> src/test/java/org/apache/aurora/scheduler/state/LockManagerImplTest.java c8ad55d8d48f7e96180846ab515dd4df3d8ed79e
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 91c1c24448092e1b3454844ab8074ed030383594
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 62fc8045f6a5fda234df73452685bd04e3142aaf
>
> Diff: https://reviews.apache.org/r/16629/diff/
>
>
> Testing
> -------
>
> gradle build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 16629: Client quota check (server side)
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Jan. 4, 2014, 12:55 a.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/state/LockManager.java, line 65
> > <https://reviews.apache.org/r/16629/diff/1/?file=415149#file415149line65>
> >
> > Is there a reason to take an Optional here? Shouldn't the caller just not call in if the provided lock is null?
>
> Maxim Khutornenko wrote:
> This is a convenience method for cases like getQuota() where Lock presence is optional as it's used in both locked (update, create) and unlocked (getQuota api) contexts. Having Optional here avoids IF condition on the SchedulerThriftInterface side.
>
> Kevin Sweeney wrote:
> I personally prefer that input sanitization happen closer to the request processing code when possible (which allows us to assume that no nulls are passed to core objects) but I'll defer to consensus here.
>
> Maxim Khutornenko wrote:
> Actually, it might even make sense to drop the Lock from getQuota() call altogether. The getQuota() does not make any changes to the system and both update and create flows would already have locks acquired by the time this RPC is called. So, validating lock context within getQuota() would not add anything. Any objections?
>
> Bill Farner wrote:
> I agree with your thought — the lock on getQuota doesn't add much currently.
Had an offline discussion with Kevin and we came to the same conclusion. There is a current resource race where different jobs of the same role would draw from the same quota pool. That race could potentially be resolved by applying a role level lock. However, we would need to have a hierarchical storage in place first to correctly implement locking anywhere higher than job level. Dropping it for now.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16629/#review31197
-----------------------------------------------------------
On Jan. 4, 2014, 12:03 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16629/
> -----------------------------------------------------------
>
> (Updated Jan. 4, 2014, 12:03 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Part 2: Server side changes for the client quota check.
>
> Note: api.thrift changes are duplicated from part 1: https://reviews.apache.org/r/16444/ and will race to submission with it.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/state/LockManager.java 5f34de1bd0f90269642ea8d451ce4cd6fe180c2d
> src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java bf0a650ae55eaa66ae7ee2b8a201cb5bda38177f
> src/main/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java 8fb51d69be6d370f9f010c797b2c1205b38a04f5
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java c1a11bdb91c5e764864324d26248d1783af8048b
> src/main/thrift/org/apache/aurora/gen/api.thrift 480b8f472bcfbe547a91c41638250350a0110334
> src/test/java/org/apache/aurora/scheduler/state/LockManagerImplTest.java c8ad55d8d48f7e96180846ab515dd4df3d8ed79e
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 91c1c24448092e1b3454844ab8074ed030383594
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 62fc8045f6a5fda234df73452685bd04e3142aaf
>
> Diff: https://reviews.apache.org/r/16629/diff/
>
>
> Testing
> -------
>
> gradle build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 16629: Client quota check (server side)
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Jan. 4, 2014, 12:55 a.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/state/LockManager.java, line 65
> > <https://reviews.apache.org/r/16629/diff/1/?file=415149#file415149line65>
> >
> > Is there a reason to take an Optional here? Shouldn't the caller just not call in if the provided lock is null?
This is a convenience method for cases like getQuota() where Lock presence is optional as it's used in both locked (update, create) and unlocked (getQuota api) contexts. Having Optional here avoids IF condition on the SchedulerThriftInterface side.
> On Jan. 4, 2014, 12:55 a.m., Kevin Sweeney wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 461
> > <https://reviews.apache.org/r/16629/diff/1/?file=415153#file415153line461>
> >
> > remove extra space
Done.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16629/#review31197
-----------------------------------------------------------
On Jan. 4, 2014, 12:03 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16629/
> -----------------------------------------------------------
>
> (Updated Jan. 4, 2014, 12:03 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Part 2: Server side changes for the client quota check.
>
> Note: api.thrift changes are duplicated from part 1: https://reviews.apache.org/r/16444/ and will race to submission with it.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/state/LockManager.java 5f34de1bd0f90269642ea8d451ce4cd6fe180c2d
> src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java bf0a650ae55eaa66ae7ee2b8a201cb5bda38177f
> src/main/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java 8fb51d69be6d370f9f010c797b2c1205b38a04f5
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java c1a11bdb91c5e764864324d26248d1783af8048b
> src/main/thrift/org/apache/aurora/gen/api.thrift 480b8f472bcfbe547a91c41638250350a0110334
> src/test/java/org/apache/aurora/scheduler/state/LockManagerImplTest.java c8ad55d8d48f7e96180846ab515dd4df3d8ed79e
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 91c1c24448092e1b3454844ab8074ed030383594
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 62fc8045f6a5fda234df73452685bd04e3142aaf
>
> Diff: https://reviews.apache.org/r/16629/diff/
>
>
> Testing
> -------
>
> gradle build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 16629: Client quota check (server side)
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16629/#review31197
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/state/LockManager.java
<https://reviews.apache.org/r/16629/#comment59598>
Is there a reason to take an Optional here? Shouldn't the caller just not call in if the provided lock is null?
src/main/thrift/org/apache/aurora/gen/api.thrift
<https://reviews.apache.org/r/16629/#comment59599>
remove extra space
- Kevin Sweeney
On Jan. 3, 2014, 4:03 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16629/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2014, 4:03 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Part 2: Server side changes for the client quota check.
>
> Note: api.thrift changes are duplicated from part 1: https://reviews.apache.org/r/16444/ and will race to submission with it.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/state/LockManager.java 5f34de1bd0f90269642ea8d451ce4cd6fe180c2d
> src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java bf0a650ae55eaa66ae7ee2b8a201cb5bda38177f
> src/main/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java 8fb51d69be6d370f9f010c797b2c1205b38a04f5
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java c1a11bdb91c5e764864324d26248d1783af8048b
> src/main/thrift/org/apache/aurora/gen/api.thrift 480b8f472bcfbe547a91c41638250350a0110334
> src/test/java/org/apache/aurora/scheduler/state/LockManagerImplTest.java c8ad55d8d48f7e96180846ab515dd4df3d8ed79e
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 91c1c24448092e1b3454844ab8074ed030383594
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 62fc8045f6a5fda234df73452685bd04e3142aaf
>
> Diff: https://reviews.apache.org/r/16629/diff/
>
>
> Testing
> -------
>
> gradle build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 16629: Client quota check (server side)
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16629/#review31202
-----------------------------------------------------------
Ship it!
Ship It!
- Kevin Sweeney
On Jan. 3, 2014, 4:03 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16629/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2014, 4:03 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Part 2: Server side changes for the client quota check.
>
> Note: api.thrift changes are duplicated from part 1: https://reviews.apache.org/r/16444/ and will race to submission with it.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/state/LockManager.java 5f34de1bd0f90269642ea8d451ce4cd6fe180c2d
> src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java bf0a650ae55eaa66ae7ee2b8a201cb5bda38177f
> src/main/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java 8fb51d69be6d370f9f010c797b2c1205b38a04f5
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java c1a11bdb91c5e764864324d26248d1783af8048b
> src/main/thrift/org/apache/aurora/gen/api.thrift 480b8f472bcfbe547a91c41638250350a0110334
> src/test/java/org/apache/aurora/scheduler/state/LockManagerImplTest.java c8ad55d8d48f7e96180846ab515dd4df3d8ed79e
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 91c1c24448092e1b3454844ab8074ed030383594
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 62fc8045f6a5fda234df73452685bd04e3142aaf
>
> Diff: https://reviews.apache.org/r/16629/diff/
>
>
> Testing
> -------
>
> gradle build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>