aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Maxim Khutornenko" <ma...@apache.org>
Subject Re: Review Request 24815: Refactoring SchedulerCore final part.
Date Thu, 04 Sep 2014 20:54:46 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message