> On Aug. 8, 2014, 4:17 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java, > > line 41 > > <https://reviews.apache.org/r/24465/diff/2/?file=655800#file655800line41> > > > > Missing @param <K> comment.
Fixed. > On Aug. 8, 2014, 4:17 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java, > > line 65 > > <https://reviews.apache.org/r/24465/diff/2/?file=655800#file655800line65> > > > > 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? Presumably events will directly correlation with actions taken on tasks, so that suggests they will be translated from InstanceActions. > On Aug. 8, 2014, 4:17 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java, > > lines 130-132 > > <https://reviews.apache.org/r/24465/diff/2/?file=655800#file655800line130> > > > > 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. Done, pulled the job-level transition out. > On Aug. 8, 2014, 4:17 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java, > > lines 170-171 > > <https://reviews.apache.org/r/24465/diff/2/?file=655800#file655800line170> > > > > 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? There are a few areas that need to be revisited for rollback, which should not be major. Added a TODO. > On Aug. 8, 2014, 4:17 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java, > > lines 237-245 > > <https://reviews.apache.org/r/24465/diff/2/?file=655800#file655800line237> > > > > I thought the thrift JobUpdateAction is what we would use here, no? Actions will be a subset of this enum. - Bill ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24465/#review50038 ----------------------------------------------------------- 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 > >
