Re: 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/#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.
--- 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.
--- 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.
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
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
