You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Bill Farner <wf...@apache.org> on 2014/07/30 03:06:25 UTC
Review Request 24078: Add an InstanceUpdater to fit into rolling update
coordination in the scheduler.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24078/
-----------------------------------------------------------
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.
Posted by Bill Farner <wf...@apache.org>.
> On July 31, 2014, 6:38 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java, line 107
> > <https://reviews.apache.org/r/24078/diff/1/?file=645380#file645380line107>
> >
> > 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
> > <https://reviews.apache.org/r/24078/diff/1/?file=645380#file645380line145>
> >
> > 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
> > <https://reviews.apache.org/r/24078/diff/1/?file=645380#file645380line150>
> >
> > 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.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
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
<https://reviews.apache.org/r/24078/#comment86165>
src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java
<https://reviews.apache.org/r/24078/#comment86194>
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
<https://reviews.apache.org/r/24078/#comment86163>
typo
src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java
<https://reviews.apache.org/r/24078/#comment86164>
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
>
>
Re: Review Request 24078: Add an InstanceUpdater to fit into rolling update
coordination in the scheduler.
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
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.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
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.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
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.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
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.
Posted by Bill Farner <wf...@apache.org>.
> 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
>
>
Re: Review Request 24078: Add an InstanceUpdater to fit into rolling update
coordination in the scheduler.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
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
<https://reviews.apache.org/r/24078/#comment86270>
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
<https://reviews.apache.org/r/24078/#comment86267>
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
<https://reviews.apache.org/r/24078/#comment86271>
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.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
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