----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24465/#review50038 -----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java <https://reviews.apache.org/r/24465/#comment87549> Missing @param <K> comment. src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java <https://reviews.apache.org/r/24465/#comment87553> How would instance events propagate from here? From what I can see the caller would not have the instance state exposure, so I am assuming it's going to be another "instanceEventSink" constructor argument? src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java <https://reviews.apache.org/r/24465/#comment87552> This method is a bit hard to follow given the presence of both update and instance state machine transitions. How about encapsulating instance transitions in helper methods? Would make the algorithm here more readable. src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java <https://reviews.apache.org/r/24465/#comment87554> This would make it impossible to do a failure-agnostic rollback we currently have in client. Are you suggesting the rollback would be a best effort aborting in case of maxFailedInstances reached? src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java <https://reviews.apache.org/r/24465/#comment87555> I thought the thrift JobUpdateAction is what we would use here, no? - Maxim Khutornenko On Aug. 7, 2014, 11:11 p.m., Bill Farner wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24465/ > ----------------------------------------------------------- > > (Updated Aug. 7, 2014, 11:11 p.m.) > > > Review request for Aurora, Kevin Sweeney and Maxim Khutornenko. > > > Bugs: AURORA-613 > https://issues.apache.org/jira/browse/AURORA-613 > > > Repository: aurora > > > Description > ------- > > There are 3 levels to performing an update: > > 1. Move the job from state A to state B, roll back on failure > 2. Take a job from state A to state B > 3. Take an instance from state A to state B > > This implements level 2. I made the OneWayJobUpdater generic, which actually > simplified both implementation and testing, since it is only responsible for > relaying that state down to level 3. > > > Diffs > ----- > > > src/main/java/org/apache/aurora/scheduler/updater/InstanceStateProvider.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java > 7476d82e9691449fa968c5fc4c5af76837a5c9cf > src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/updater/StateEvaluator.java > PRE-CREATION > src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java > dda1b73ead847e5ee9c5c7bc8be3cd8a7f59ac80 > src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/24465/diff/ > > > Testing > ------- > > ./gradlew build -Pq > > OneWayJobUpdater has 100% instruction and branch coverage. > > > Thanks, > > Bill Farner > >
