Re: Review Request 24078: Add an InstanceUpdater to fit into rolling update coordination in the scheduler.

[David McLaughlin]

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

Ship it!


Ship It!

- David McLaughlin


On Aug. 1, 2014, 9:50 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24078/
> ---
> 
> (Updated Aug. 1, 2014, 9:50 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
> 
>

Re: Review Request 24078: Add an InstanceUpdater to fit into rolling update coordination in the scheduler.

[Kevin Sweeney]

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

Ship it!


Ship It!

- Kevin Sweeney


On Aug. 1, 2014, 2:50 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24078/
> ---
> 
> (Updated Aug. 1, 2014, 2:50 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
> 
>

Re: Review Request 24078: Add an InstanceUpdater to fit into rolling update coordination in the scheduler.

[Maxim Khutornenko]

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

Ship it!


Ship It!

- Maxim Khutornenko


On Aug. 1, 2014, 9:50 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24078/
> ---
> 
> (Updated Aug. 1, 2014, 9:50 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
> 
>

Re: Review Request 24078: Add an InstanceUpdater to fit into rolling update coordination in the scheduler.

[Bill Farner]

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

(Updated Aug. 1, 2014, 9:50 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 (updated)
-

  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

Re: Review Request 24078: Add an InstanceUpdater to fit into rolling update coordination in the scheduler.

[Bill Farner]

> On Aug. 1, 2014, 12:20 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/TaskController.java, line 
> > 32
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> 
>

Re: Review Request 24078: Add an InstanceUpdater to fit into rolling update coordination in the scheduler.

[Maxim Khutornenko]

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



src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java


Not sure I get the logic here. If addFailure() is True, meaning the 
instance has FAILED already, why do we want to reevaluateAfterRunningLimit()?



src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java


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?



src/main/java/org/apache/aurora/scheduler/updater/TaskController.java


typo


- Maxim Khutornenko


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

Re: Review Request 24078: Add an InstanceUpdater to fit into rolling update coordination in the scheduler.

[Bill Farner]

> On July 31, 2014, 6:38 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java, 
> > line 107
> > 
> >
> > It seems weird and error-prone to discard the reference when the 
> > interface requires that calls post-complete are noops. Consider just making 
> > this field final and avoiding the gymnastics here.

Minor correction, post-complete calls are no-ops _as part of InstanceUpdater's 
contract_.  Are you saying that you would rather the black-hole be engaged on 
the TaskController side?


> On July 31, 2014, 6:38 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java, 
> > line 145
> > 
> >
> > typo

fixed


> On July 31, 2014, 6:38 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java, 
> > line 150
> > 
> >
> > typo

fixed


- Bill


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


On July 30, 2014, 1:06 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24078/
> ---
> 
> (Updated July 30, 2014, 1:06 a.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
> 
>

Re: Review Request 24078: Add an InstanceUpdater to fit into rolling update coordination in the scheduler.

[Bill Farner]

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


Changes
---

fixed typos


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 (updated)
-

  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

Re: Review Request 24078: Add an InstanceUpdater to fit into rolling update coordination in the scheduler.

[Kevin Sweeney]

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



src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java






src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java


It seems weird and error-prone to discard the reference when the interface 
requires that calls post-complete are noops. Consider just making this field 
final and avoiding the gymnastics here.



src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java


typo



src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java


typo


- Kevin Sweeney


On July 29, 2014, 6:06 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24078/
> ---
> 
> (Updated July 29, 2014, 6:06 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
> 
>