> On Aug. 1, 2014, 12:20 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/TaskController.java, line 
> > 32
> > <https://reviews.apache.org/r/24078/diff/2/?file=647481#file647481line32>
> >
> >     typo

Fixed.


> On Aug. 1, 2014, 12:20 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java, 
> > lines 203-206
> > <https://reviews.apache.org/r/24078/diff/2/?file=647480#file647480line203>
> >
> >     Should it be restricted to only KILLED tasks? I.e. is it possible that 
> > a LOST task processed here results in adding extra instance in addition to 
> > a rescheduled one?

I think you have the right idea, but the details are slightly different.  We 
should check for whether the task is terminated and has passed through KILLING, 
which means it will not be resuscitated.  I've added a test case to catch the 
logic flaw, and fixed it.


> On Aug. 1, 2014, 12:20 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java, 
> > lines 189-193
> > <https://reviews.apache.org/r/24078/diff/2/?file=647480#file647480line189>
> >
> >     Not sure I get the logic here. If addFailure() is True, meaning the 
> > instance has FAILED already, why do we want to 
> > reevaluateAfterRunningLimit()?

Good catch, this was bad logic.  Fixed in latest diff.


- Bill


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


On July 31, 2014, 11:52 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24078/
> -----------------------------------------------------------
> 
> (Updated July 31, 2014, 11:52 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Kevin Sweeney, and Maxim 
> Khutornenko.
> 
> 
> Bugs: AURORA-621
>     https://issues.apache.org/jira/browse/AURORA-621
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Note: this is one part of the epic AURORA-610, and is currently unwired code.
> 
> This implements the logic to initiate a change from a possibly-absent current 
> task state to a possibly-absent desired state.  I worked with Maxim on this, 
> and we started with a state machine approach.  After some more careful 
> thought, i pushed for a convergence function, which is implemented here.
> 
> There are a few invariants assumed/designed here, which i've called out in 
> the javadoc for evaluate().
> 
> The caller of this function will be responsible for implementing the 
> TaskController interface.  As for integration, this will mean:
>   - maintaining one InstanceUpdater for each in-flight instance update
>   - subscribing to task pubsub events and routing them to the appropriate 
> InstanceUpdater's evaluate()
>   - discarding terminal InstanceUpdaters (those which have called 
> TaskController#updateCompleted())
>   - scheduling delayed calls to evaluate() based on the job's update 
> parameters
> 
> My advice for reviewing is to first scan the evaluate() function (without 
> skipping out to helper functions), then dive into 
> handleActualAndDesiredPresent().  Think of evaluate() as something that may 
> be called arbitrarily, and attempts to gracefully move a task from the actual 
> state to the desired state.  Next, look at the unit test to see how the calls 
> should look to turn this function into an event-driven convergence function.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/updater/TaskController.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/24078/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew build -Pq, 100% instruction and branch test coverage.
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Reply via email to