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