Re: Review Request 24465: Add a one-way job update controller.
On Aug. 7, 2014, 11:24 p.m., Kevin Sweeney wrote: src/main/java/org/apache/aurora/scheduler/updater/StateEvaluator.java, lines 17-18 https://reviews.apache.org/r/24465/diff/2/?file=655801#file655801line17 It's unclear to me why T is needed here. Also, this description seems too abstract (to the point where I'm not sure what is being described here). Yeah, i was concerned about it being so abstract that it's confusing. I'll try to re-word this doc. T is nice since it proves that OneWayJobUpdater is not coupled to the state type (IScheduledTask), which has the nicer side-effect of testing using a simple type (string). I considered using Function instead, but didn't want the baggage of @Nullable outputs. On Aug. 7, 2014, 11:24 p.m., Kevin Sweeney wrote: src/main/java/org/apache/aurora/scheduler/updater/StateEvaluator.java, lines 17-20 https://reviews.apache.org/r/24465/diff/2/?file=655801#file655801line17 Not clear why T is needed here. Also, the description seems too abstract, maybe add an example of what this might be used for? Assuming this is a dupe comment of the above. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24465/#review49969 --- 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
Re: Review Request 24465: Add a one-way job update controller.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24465/#review50232 --- Ship it! src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java https://reviews.apache.org/r/24465/#comment87877 There is still job/instance state update mix here. What I was trying to propose is a set of action methods like: ... reEvaluateInstances(); beginUpdatingNewInstances(); computeJobUpdateStatus(); - Maxim Khutornenko On Aug. 11, 2014, 8:19 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24465/ --- (Updated Aug. 11, 2014, 8:19 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
Re: Review Request 24465: Add a one-way job update controller.
On Aug. 11, 2014, 9:05 p.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java, lines 135-137 https://reviews.apache.org/r/24465/diff/2-3/?file=655800#file655800line135 There is still job/instance state update mix here. What I was trying to propose is a set of action methods like: ... reEvaluateInstances(); beginUpdatingNewInstances(); computeJobUpdateStatus(); Took a stab at this. To be honest, i think it reduces readability, but we can revisit later if necessary. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24465/#review50232 --- On Aug. 11, 2014, 8:19 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24465/ --- (Updated Aug. 11, 2014, 8:19 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
Re: Review Request 24465: Add a one-way job update controller.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24465/ --- (Updated Aug. 11, 2014, 9:29 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 (updated) - 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
Re: Review Request 24465: Add a one-way job update controller.
--- 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
Re: Review Request 24465: Add a one-way job update controller.
--- 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. Changes --- Small interface tweaks - made the key identifier type generic, collapsed the two evaluate functions into one. 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 (updated) - 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
