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