> On Sept. 5, 2014, 3:25 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/state/StateManager.java, line 73
> > <https://reviews.apache.org/r/24815/diff/2/?file=679135#file679135line73>
> >
> >     "Inserts pending instances using {@code task} as their configuration.  
> > Tasks will..."

Done.


> On Sept. 5, 2014, 3:25 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java, 
> > line 35
> > <https://reviews.apache.org/r/24815/diff/2/?file=679138#file679138line35>
> >
> >     s/task specific/task-specific/

Done.


> On Sept. 5, 2014, 3:25 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java, 
> > line 40
> > <https://reviews.apache.org/r/24815/diff/2/?file=679138#file679138line40>
> >
> >     Please phrase this in terms of what this specific method is responsible 
> > for.  Storage and transaction details should be out of scope.

Good catch. Refactoring copypasta.


> On Sept. 5, 2014, 3:25 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java, 
> > line 47
> > <https://reviews.apache.org/r/24815/diff/2/?file=679138#file679138line47>
> >
> >     s/instances/newInstances/?
> >     
> >     It would be nice for the parameter name and doc to call out that the 
> > validation is based on things being _added_ to the existing task pool.

| It would be nice for the parameter name and doc to call out that the 
validation is based on things being added to the existing task pool.

This is not entirely true. It's also called from createJob() when the task pool 
techincally does not exist yet. 

However, your comment points to a potential flaw with MAX_TASKS_PER_JOB check. 
It does not cover the addInstances case. It's currently possible to create an 
unbounded number of job instances via subsequent job updates with newInstances 
<= MAX_TASKS_PER_JOB. Added a TODO and will address this in a follow up review.


> On Sept. 5, 2014, 3:25 p.m., Bill Farner wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 304
> > <https://reviews.apache.org/r/24815/diff/2/?file=679140#file679140line304>
> >
> >     Good catch!  In a follow-up review, can you kick off the deprecation?  
> > I've yet to open a discussion about this process and document it, but an 
> > example is here: https://issues.apache.org/jira/browse/AURORA-423

Sure. Will do along with the MAX_TASKS_PER_JOB issue mentioned above.


> On Sept. 5, 2014, 3:25 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/state/TaskLimitValidatorTest.java,
> >  line 54
> > <https://reviews.apache.org/r/24815/diff/2/?file=679145#file679145line54>
> >
> >     s/ throws Exception//?

Dropped.


- Maxim


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24815/#review52455
-----------------------------------------------------------


On Sept. 5, 2014, 12:09 a.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24815/
> -----------------------------------------------------------
> 
> (Updated Sept. 5, 2014, 12:09 a.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/configuration/SanitizedConfiguration.java
>  d511ec0054e380fd28b0c70bae7d9803b81abdf1 
>   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 
> 1e60fcad82b05e3fe477a31e653b8c5e63c78050 
>   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 
> 2c712eff097c3334bfcf2559a52214367748d08a 
>   src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  9171179ff482e0b56faf4073b2dc2a6f2f0def46 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> a78a4d849e545000725a39fae49f406422c39da0 
>   
> 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
>  b22b390368e08d3790480ab5505852f398b2c69c 
>   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/StorageBackfillTest.java 
> ad2548c53d86b89efe6129702b8a5df259af3d4e 
>   
> src/test/java/org/apache/aurora/scheduler/storage/mem/MemStorageSchedulerCoreImplTest.java
>  35bed104d838596abcbb5abd5cad29592b384dfa 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  1686d4b158057d87180af3a211a4237f1213efa6 
>   src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java 
> 055c177bc34cc306faaf7593e6c5156ad9636f6c 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 81d1734a22a8744d6002aadb7fb446d132d10bd9 
> 
> Diff: https://reviews.apache.org/r/24815/diff/
> 
> 
> Testing
> -------
> 
> gradle -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Reply via email to