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

Reply via email to