You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Bill Farner <wf...@apache.org> on 2014/09/03 00:36:56 UTC
Re: Review Request 24815: Refactoring SchedulerCore final part.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24815/#review52089
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java
<https://reviews.apache.org/r/24815/#comment90823>
At first glance, this is odd since ITaskConfig contains the components of a JobKey. Is that argument necessary?
src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java
<https://reviews.apache.org/r/24815/#comment90835>
Is this interface useful? The implementation logic is trivial, and it's only used from SchedulerThriftInterface. I suggest putting it there.
- Bill Farner
On Aug. 18, 2014, 8:04 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24815/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2014, 8:04 p.m.)
>
>
> Review request for Aurora and Bill Farner.
>
>
> Bugs: AURORA-94
> https://issues.apache.org/jira/browse/AURORA-94
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Moving the last bits of functionality out of SchedulerCore.
>
> Also, preparing the ground for task validation logic reuse in updater code.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/base/ScheduleException.java e060e5ec88a0ab311415eaa638cf693c99c40049
> src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java 62202810fd5829545fc77b91a20d1bb433a4916a
> src/main/java/org/apache/aurora/scheduler/state/SchedulerCore.java c636fd7fde8b592b167da8e5e9651ac772bc23de
> src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 3dcb1c3e3d0138634b3d077c845ecc0d61d7fc0f
> src/main/java/org/apache/aurora/scheduler/state/StateManager.java 6e062b39175255264020bbb1cbd485de9a114a20
> src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 6ad104b050aabecad1dadc975c27d9d3603659bf
> src/main/java/org/apache/aurora/scheduler/state/StateModule.java cc1eee4e19c092c0d401558ac01b54627f2d1290
> src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 7ef28858ad290c74248b89c49d2a684eb1c7127e
> src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java c7ae7db16e82c722fae1bccb9178f5ec203e91e5
> src/test/java/org/apache/aurora/scheduler/http/JettyServerModuleTest.java 62dce07b42af02d25b788e763e4e2a026dd2d483
> src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java fa611a913bad40a8c0515c578b394c460340e574
> src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 1678411ad5c25e74f8bbbbb6d004bf491ea26ca0
> src/test/java/org/apache/aurora/scheduler/state/TaskLimitValidatorTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/mem/MemStorageSchedulerCoreImplTest.java 35bed104d838596abcbb5abd5cad29592b384dfa
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 649afa24b2cfc9a1d67d350473e439d209bd720c
> src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java 43265fdab1ae900fb828374f6c69e562def2d682
>
> Diff: https://reviews.apache.org/r/24815/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24815: Refactoring SchedulerCore final part.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Sept. 2, 2014, 10:36 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java, line 120
> > <https://reviews.apache.org/r/24815/diff/1/?file=662756#file662756line120>
> >
> > At first glance, this is odd since ITaskConfig contains the components of a JobKey. Is that argument necessary?
I tried and really hated that approach. Adding a bit of redunancy helped avoiding clumsy job key extraction and edge case error conditions (e.g. empty collection, ITaskConfigs from different jobs and etc.)
> On Sept. 2, 2014, 10:36 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java, line 35
> > <https://reviews.apache.org/r/24815/diff/1/?file=662758#file662758line35>
> >
> > Is this interface useful? The implementation logic is trivial, and it's only used from SchedulerThriftInterface. I suggest putting it there.
Right now that is correct. However, I'd expect JobUpdateController to accept it in order to check quota any time an instance about to be added.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24815/#review52089
-----------------------------------------------------------
On Aug. 18, 2014, 8:04 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24815/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2014, 8:04 p.m.)
>
>
> Review request for Aurora and Bill Farner.
>
>
> Bugs: AURORA-94
> https://issues.apache.org/jira/browse/AURORA-94
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Moving the last bits of functionality out of SchedulerCore.
>
> Also, preparing the ground for task validation logic reuse in updater code.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/base/ScheduleException.java e060e5ec88a0ab311415eaa638cf693c99c40049
> src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java 62202810fd5829545fc77b91a20d1bb433a4916a
> src/main/java/org/apache/aurora/scheduler/state/SchedulerCore.java c636fd7fde8b592b167da8e5e9651ac772bc23de
> src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 3dcb1c3e3d0138634b3d077c845ecc0d61d7fc0f
> src/main/java/org/apache/aurora/scheduler/state/StateManager.java 6e062b39175255264020bbb1cbd485de9a114a20
> src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 6ad104b050aabecad1dadc975c27d9d3603659bf
> src/main/java/org/apache/aurora/scheduler/state/StateModule.java cc1eee4e19c092c0d401558ac01b54627f2d1290
> src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 7ef28858ad290c74248b89c49d2a684eb1c7127e
> src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java c7ae7db16e82c722fae1bccb9178f5ec203e91e5
> src/test/java/org/apache/aurora/scheduler/http/JettyServerModuleTest.java 62dce07b42af02d25b788e763e4e2a026dd2d483
> src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java fa611a913bad40a8c0515c578b394c460340e574
> src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 1678411ad5c25e74f8bbbbb6d004bf491ea26ca0
> src/test/java/org/apache/aurora/scheduler/state/TaskLimitValidatorTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/mem/MemStorageSchedulerCoreImplTest.java 35bed104d838596abcbb5abd5cad29592b384dfa
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 649afa24b2cfc9a1d67d350473e439d209bd720c
> src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java 43265fdab1ae900fb828374f6c69e562def2d682
>
> Diff: https://reviews.apache.org/r/24815/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24815: Refactoring SchedulerCore final part.
Posted by Bill Farner <wf...@apache.org>.
> On Sept. 2, 2014, 10:36 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java, line 120
> > <https://reviews.apache.org/r/24815/diff/1/?file=662756#file662756line120>
> >
> > At first glance, this is odd since ITaskConfig contains the components of a JobKey. Is that argument necessary?
>
> Maxim Khutornenko wrote:
> I tried and really hated that approach. Adding a bit of redunancy helped avoiding clumsy job key extraction and edge case error conditions (e.g. empty collection, ITaskConfigs from different jobs and etc.)
> avoiding clumsy job key extraction and edge case error conditions
Those cases are still there though, right? Except now there's an additional case currently not handled where the keys don't match. Perhaps the better approach is to change the signature to:
`insertPendingTasks(ITaskConfig config, Set<Integer> instanceIds);`
Looking at the call sites, this is how it's used in practice.
In fact, there's a relevant TODO in SanitizedConfiguration:
`// TODO(William Farner): Rework this API now that all configs are identical.`
Even the late SchedulerCore#addInstance took this approach:
```java
public void addInstances(
final IJobKey jobKey,
final ImmutableSet<Integer> instanceIds,
final ITaskConfig config) throws ScheduleException {
```
(though unfortunately it also duplicated the job key)
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24815/#review52089
-----------------------------------------------------------
On Aug. 18, 2014, 8:04 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24815/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2014, 8:04 p.m.)
>
>
> Review request for Aurora and Bill Farner.
>
>
> Bugs: AURORA-94
> https://issues.apache.org/jira/browse/AURORA-94
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Moving the last bits of functionality out of SchedulerCore.
>
> Also, preparing the ground for task validation logic reuse in updater code.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/base/ScheduleException.java e060e5ec88a0ab311415eaa638cf693c99c40049
> src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java 62202810fd5829545fc77b91a20d1bb433a4916a
> src/main/java/org/apache/aurora/scheduler/state/SchedulerCore.java c636fd7fde8b592b167da8e5e9651ac772bc23de
> src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 3dcb1c3e3d0138634b3d077c845ecc0d61d7fc0f
> src/main/java/org/apache/aurora/scheduler/state/StateManager.java 6e062b39175255264020bbb1cbd485de9a114a20
> src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 6ad104b050aabecad1dadc975c27d9d3603659bf
> src/main/java/org/apache/aurora/scheduler/state/StateModule.java cc1eee4e19c092c0d401558ac01b54627f2d1290
> src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 7ef28858ad290c74248b89c49d2a684eb1c7127e
> src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java c7ae7db16e82c722fae1bccb9178f5ec203e91e5
> src/test/java/org/apache/aurora/scheduler/http/JettyServerModuleTest.java 62dce07b42af02d25b788e763e4e2a026dd2d483
> src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java fa611a913bad40a8c0515c578b394c460340e574
> src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 1678411ad5c25e74f8bbbbb6d004bf491ea26ca0
> src/test/java/org/apache/aurora/scheduler/state/TaskLimitValidatorTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/mem/MemStorageSchedulerCoreImplTest.java 35bed104d838596abcbb5abd5cad29592b384dfa
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 649afa24b2cfc9a1d67d350473e439d209bd720c
> src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java 43265fdab1ae900fb828374f6c69e562def2d682
>
> Diff: https://reviews.apache.org/r/24815/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24815: Refactoring SchedulerCore final part.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Sept. 2, 2014, 10:36 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java, line 35
> > <https://reviews.apache.org/r/24815/diff/1/?file=662758#file662758line35>
> >
> > Is this interface useful? The implementation logic is trivial, and it's only used from SchedulerThriftInterface. I suggest putting it there.
>
> Maxim Khutornenko wrote:
> Right now that is correct. However, I'd expect JobUpdateController to accept it in order to check quota any time an instance about to be added.
>
> Bill Farner wrote:
> > in order to check quota any time an instance about to be added
>
> Is there a situation i'm not thinking of where that is needed rather than performing the quota check only once when first initiating the update? Is this because QuotaManager doesn't account for in-progress job udpates?
Right. Since we are tracking quota at the role level but the update lock applied at the job level, there is always a possibility to exceed the allowed quota for long running updates. This is especially a problem with the server side-dirven process where a resumed update will restart in a potentially quite different quota environment (i.e. due to other jobs created while the update was paused). We could check quota whithin resumeUpdate() RPC but that would still leave room for a race. The only correct approach would be to check quota within the same transaction the "addInstance" action takes place.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24815/#review52089
-----------------------------------------------------------
On Aug. 18, 2014, 8:04 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24815/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2014, 8:04 p.m.)
>
>
> Review request for Aurora and Bill Farner.
>
>
> Bugs: AURORA-94
> https://issues.apache.org/jira/browse/AURORA-94
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Moving the last bits of functionality out of SchedulerCore.
>
> Also, preparing the ground for task validation logic reuse in updater code.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/base/ScheduleException.java e060e5ec88a0ab311415eaa638cf693c99c40049
> src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java 62202810fd5829545fc77b91a20d1bb433a4916a
> src/main/java/org/apache/aurora/scheduler/state/SchedulerCore.java c636fd7fde8b592b167da8e5e9651ac772bc23de
> src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 3dcb1c3e3d0138634b3d077c845ecc0d61d7fc0f
> src/main/java/org/apache/aurora/scheduler/state/StateManager.java 6e062b39175255264020bbb1cbd485de9a114a20
> src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 6ad104b050aabecad1dadc975c27d9d3603659bf
> src/main/java/org/apache/aurora/scheduler/state/StateModule.java cc1eee4e19c092c0d401558ac01b54627f2d1290
> src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 7ef28858ad290c74248b89c49d2a684eb1c7127e
> src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java c7ae7db16e82c722fae1bccb9178f5ec203e91e5
> src/test/java/org/apache/aurora/scheduler/http/JettyServerModuleTest.java 62dce07b42af02d25b788e763e4e2a026dd2d483
> src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java fa611a913bad40a8c0515c578b394c460340e574
> src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 1678411ad5c25e74f8bbbbb6d004bf491ea26ca0
> src/test/java/org/apache/aurora/scheduler/state/TaskLimitValidatorTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/mem/MemStorageSchedulerCoreImplTest.java 35bed104d838596abcbb5abd5cad29592b384dfa
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 649afa24b2cfc9a1d67d350473e439d209bd720c
> src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java 43265fdab1ae900fb828374f6c69e562def2d682
>
> Diff: https://reviews.apache.org/r/24815/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24815: Refactoring SchedulerCore final part.
Posted by Bill Farner <wf...@apache.org>.
> On Sept. 2, 2014, 10:36 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java, line 35
> > <https://reviews.apache.org/r/24815/diff/1/?file=662758#file662758line35>
> >
> > Is this interface useful? The implementation logic is trivial, and it's only used from SchedulerThriftInterface. I suggest putting it there.
>
> Maxim Khutornenko wrote:
> Right now that is correct. However, I'd expect JobUpdateController to accept it in order to check quota any time an instance about to be added.
>
> Bill Farner wrote:
> > in order to check quota any time an instance about to be added
>
> Is there a situation i'm not thinking of where that is needed rather than performing the quota check only once when first initiating the update? Is this because QuotaManager doesn't account for in-progress job udpates?
>
> Maxim Khutornenko wrote:
> Right. Since we are tracking quota at the role level but the update lock applied at the job level, there is always a possibility to exceed the allowed quota for long running updates. This is especially a problem with the server side-dirven process where a resumed update will restart in a potentially quite different quota environment (i.e. due to other jobs created while the update was paused). We could check quota whithin resumeUpdate() RPC but that would still leave room for a race. The only correct approach would be to check quota within the same transaction the "addInstance" action takes place.
That makes for a pretty crummy user experience, though, since there's nothing preventing creation of a new job and blowing up updates. That apparently is the case today, but i think it warrants a bug - can you file?
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24815/#review52089
-----------------------------------------------------------
On Aug. 18, 2014, 8:04 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24815/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2014, 8:04 p.m.)
>
>
> Review request for Aurora and Bill Farner.
>
>
> Bugs: AURORA-94
> https://issues.apache.org/jira/browse/AURORA-94
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Moving the last bits of functionality out of SchedulerCore.
>
> Also, preparing the ground for task validation logic reuse in updater code.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/base/ScheduleException.java e060e5ec88a0ab311415eaa638cf693c99c40049
> src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java 62202810fd5829545fc77b91a20d1bb433a4916a
> src/main/java/org/apache/aurora/scheduler/state/SchedulerCore.java c636fd7fde8b592b167da8e5e9651ac772bc23de
> src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 3dcb1c3e3d0138634b3d077c845ecc0d61d7fc0f
> src/main/java/org/apache/aurora/scheduler/state/StateManager.java 6e062b39175255264020bbb1cbd485de9a114a20
> src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 6ad104b050aabecad1dadc975c27d9d3603659bf
> src/main/java/org/apache/aurora/scheduler/state/StateModule.java cc1eee4e19c092c0d401558ac01b54627f2d1290
> src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 7ef28858ad290c74248b89c49d2a684eb1c7127e
> src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java c7ae7db16e82c722fae1bccb9178f5ec203e91e5
> src/test/java/org/apache/aurora/scheduler/http/JettyServerModuleTest.java 62dce07b42af02d25b788e763e4e2a026dd2d483
> src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java fa611a913bad40a8c0515c578b394c460340e574
> src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 1678411ad5c25e74f8bbbbb6d004bf491ea26ca0
> src/test/java/org/apache/aurora/scheduler/state/TaskLimitValidatorTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/mem/MemStorageSchedulerCoreImplTest.java 35bed104d838596abcbb5abd5cad29592b384dfa
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 649afa24b2cfc9a1d67d350473e439d209bd720c
> src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java 43265fdab1ae900fb828374f6c69e562def2d682
>
> Diff: https://reviews.apache.org/r/24815/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24815: Refactoring SchedulerCore final part.
Posted by Bill Farner <wf...@apache.org>.
> On Sept. 2, 2014, 10:36 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java, line 35
> > <https://reviews.apache.org/r/24815/diff/1/?file=662758#file662758line35>
> >
> > Is this interface useful? The implementation logic is trivial, and it's only used from SchedulerThriftInterface. I suggest putting it there.
>
> Maxim Khutornenko wrote:
> Right now that is correct. However, I'd expect JobUpdateController to accept it in order to check quota any time an instance about to be added.
> in order to check quota any time an instance about to be added
Is there a situation i'm not thinking of where that is needed rather than performing the quota check only once when first initiating the update? Is this because QuotaManager doesn't account for in-progress job udpates?
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24815/#review52089
-----------------------------------------------------------
On Aug. 18, 2014, 8:04 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24815/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2014, 8:04 p.m.)
>
>
> Review request for Aurora and Bill Farner.
>
>
> Bugs: AURORA-94
> https://issues.apache.org/jira/browse/AURORA-94
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Moving the last bits of functionality out of SchedulerCore.
>
> Also, preparing the ground for task validation logic reuse in updater code.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/base/ScheduleException.java e060e5ec88a0ab311415eaa638cf693c99c40049
> src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java 62202810fd5829545fc77b91a20d1bb433a4916a
> src/main/java/org/apache/aurora/scheduler/state/SchedulerCore.java c636fd7fde8b592b167da8e5e9651ac772bc23de
> src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 3dcb1c3e3d0138634b3d077c845ecc0d61d7fc0f
> src/main/java/org/apache/aurora/scheduler/state/StateManager.java 6e062b39175255264020bbb1cbd485de9a114a20
> src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 6ad104b050aabecad1dadc975c27d9d3603659bf
> src/main/java/org/apache/aurora/scheduler/state/StateModule.java cc1eee4e19c092c0d401558ac01b54627f2d1290
> src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 7ef28858ad290c74248b89c49d2a684eb1c7127e
> src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java c7ae7db16e82c722fae1bccb9178f5ec203e91e5
> src/test/java/org/apache/aurora/scheduler/http/JettyServerModuleTest.java 62dce07b42af02d25b788e763e4e2a026dd2d483
> src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java fa611a913bad40a8c0515c578b394c460340e574
> src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 1678411ad5c25e74f8bbbbb6d004bf491ea26ca0
> src/test/java/org/apache/aurora/scheduler/state/TaskLimitValidatorTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/mem/MemStorageSchedulerCoreImplTest.java 35bed104d838596abcbb5abd5cad29592b384dfa
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 649afa24b2cfc9a1d67d350473e439d209bd720c
> src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java 43265fdab1ae900fb828374f6c69e562def2d682
>
> Diff: https://reviews.apache.org/r/24815/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24815: Refactoring SchedulerCore final part.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Sept. 2, 2014, 10:36 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java, line 35
> > <https://reviews.apache.org/r/24815/diff/1/?file=662758#file662758line35>
> >
> > Is this interface useful? The implementation logic is trivial, and it's only used from SchedulerThriftInterface. I suggest putting it there.
>
> Maxim Khutornenko wrote:
> Right now that is correct. However, I'd expect JobUpdateController to accept it in order to check quota any time an instance about to be added.
>
> Bill Farner wrote:
> > in order to check quota any time an instance about to be added
>
> Is there a situation i'm not thinking of where that is needed rather than performing the quota check only once when first initiating the update? Is this because QuotaManager doesn't account for in-progress job udpates?
>
> Maxim Khutornenko wrote:
> Right. Since we are tracking quota at the role level but the update lock applied at the job level, there is always a possibility to exceed the allowed quota for long running updates. This is especially a problem with the server side-dirven process where a resumed update will restart in a potentially quite different quota environment (i.e. due to other jobs created while the update was paused). We could check quota whithin resumeUpdate() RPC but that would still leave room for a race. The only correct approach would be to check quota within the same transaction the "addInstance" action takes place.
>
> Bill Farner wrote:
> That makes for a pretty crummy user experience, though, since there's nothing preventing creation of a new job and blowing up updates. That apparently is the case today, but i think it warrants a bug - can you file?
https://issues.apache.org/jira/browse/AURORA-686
> On Sept. 2, 2014, 10:36 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java, line 120
> > <https://reviews.apache.org/r/24815/diff/1/?file=662756#file662756line120>
> >
> > At first glance, this is odd since ITaskConfig contains the components of a JobKey. Is that argument necessary?
>
> Maxim Khutornenko wrote:
> I tried and really hated that approach. Adding a bit of redunancy helped avoiding clumsy job key extraction and edge case error conditions (e.g. empty collection, ITaskConfigs from different jobs and etc.)
>
> Bill Farner wrote:
> > avoiding clumsy job key extraction and edge case error conditions
>
> Those cases are still there though, right? Except now there's an additional case currently not handled where the keys don't match. Perhaps the better approach is to change the signature to:
>
> `insertPendingTasks(ITaskConfig config, Set<Integer> instanceIds);`
>
> Looking at the call sites, this is how it's used in practice.
>
> In fact, there's a relevant TODO in SanitizedConfiguration:
>
> `// TODO(William Farner): Rework this API now that all configs are identical.`
>
> Even the late SchedulerCore#addInstance took this approach:
>
>
> ```java
> public void addInstances(
> final IJobKey jobKey,
> final ImmutableSet<Integer> instanceIds,
> final ITaskConfig config) throws ScheduleException {
> ```
>
> (though unfortunately it also duplicated the job key)
Sounds like a reasonable tradeoff. Will give it a shot.
| (though unfortunately it also duplicated the job key)
I am less concerned about that as it can be viewed as a convenience feature.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24815/#review52089
-----------------------------------------------------------
On Aug. 18, 2014, 8:04 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24815/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2014, 8:04 p.m.)
>
>
> Review request for Aurora and Bill Farner.
>
>
> Bugs: AURORA-94
> https://issues.apache.org/jira/browse/AURORA-94
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Moving the last bits of functionality out of SchedulerCore.
>
> Also, preparing the ground for task validation logic reuse in updater code.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/base/ScheduleException.java e060e5ec88a0ab311415eaa638cf693c99c40049
> src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java 62202810fd5829545fc77b91a20d1bb433a4916a
> src/main/java/org/apache/aurora/scheduler/state/SchedulerCore.java c636fd7fde8b592b167da8e5e9651ac772bc23de
> src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 3dcb1c3e3d0138634b3d077c845ecc0d61d7fc0f
> src/main/java/org/apache/aurora/scheduler/state/StateManager.java 6e062b39175255264020bbb1cbd485de9a114a20
> src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 6ad104b050aabecad1dadc975c27d9d3603659bf
> src/main/java/org/apache/aurora/scheduler/state/StateModule.java cc1eee4e19c092c0d401558ac01b54627f2d1290
> src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 7ef28858ad290c74248b89c49d2a684eb1c7127e
> src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java c7ae7db16e82c722fae1bccb9178f5ec203e91e5
> src/test/java/org/apache/aurora/scheduler/http/JettyServerModuleTest.java 62dce07b42af02d25b788e763e4e2a026dd2d483
> src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java fa611a913bad40a8c0515c578b394c460340e574
> src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 1678411ad5c25e74f8bbbbb6d004bf491ea26ca0
> src/test/java/org/apache/aurora/scheduler/state/TaskLimitValidatorTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/mem/MemStorageSchedulerCoreImplTest.java 35bed104d838596abcbb5abd5cad29592b384dfa
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 649afa24b2cfc9a1d67d350473e439d209bd720c
> src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java 43265fdab1ae900fb828374f6c69e562def2d682
>
> Diff: https://reviews.apache.org/r/24815/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>