Re: Review Request 25356: Add a factory to produce OneWayUpdate instances based on job state and update settings.
On Sept. 4, 2014, 10:17 p.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java, line 121 https://reviews.apache.org/r/25356/diff/1/?file=679020#file679020line121 Does it have to be synchronized? Could not find the reason based on its call chain. This was reflex, using common locking for exposed methods. This is an immutable map, though, so i'll remove it here. On Sept. 4, 2014, 10:17 p.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java, lines 94-96 https://reviews.apache.org/r/25356/diff/1/?file=679021#file679021line94 If I am reading it right the update will still proceed in case updateOnlyTheseInstances specifies an unrecognized range (i.e. not intersecting with the full working set). The InstanceUpdater appears to handle the optional desiredState as no-op. This is the departure from the current implementation where a disjoint --shards value fails the update. I think it's best for that to happen higher in the stack. The thrift interface is probably the best place for this, to validate a JobUpdateConfiguration when it's received. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25356/#review52363 --- On Sept. 4, 2014, 9:19 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25356/ --- (Updated Sept. 4, 2014, 9:19 p.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-613 https://issues.apache.org/jira/browse/AURORA-613 Repository: aurora Description --- This is the bridge between the top-level updater logic and the OneWayJobUpdater - taking a user request and the direction we've decided to move the job (forward or back), produce the appropriate OneWayJobUpdater. Diffs - src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java 85196af38b6615e5aac7de30a4a601350e935c72 src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java aa8b5f2ee81d42db003f27e682f79e94b6625d87 src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/updater/StateEvaluator.java dca55ef5612105c9dc8fd57e789fdf1df28ba447 src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java d7baaa679289998a9ea80c0934990a49e5c173d7 src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterTest.java e3e50d7d6526e8796dc5cd82b459190b09ceb72f Diff: https://reviews.apache.org/r/25356/diff/ Testing --- ./gradlew build -Pq Thanks, Bill Farner
Re: Review Request 25356: Add a factory to produce OneWayUpdate instances based on job state and update settings.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25356/ --- (Updated Sept. 5, 2014, 5:52 p.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-613 https://issues.apache.org/jira/browse/AURORA-613 Repository: aurora Description --- This is the bridge between the top-level updater logic and the OneWayJobUpdater - taking a user request and the direction we've decided to move the job (forward or back), produce the appropriate OneWayJobUpdater. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java 85196af38b6615e5aac7de30a4a601350e935c72 src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java aa8b5f2ee81d42db003f27e682f79e94b6625d87 src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/updater/StateEvaluator.java dca55ef5612105c9dc8fd57e789fdf1df28ba447 src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java d7baaa679289998a9ea80c0934990a49e5c173d7 src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterTest.java e3e50d7d6526e8796dc5cd82b459190b09ceb72f Diff: https://reviews.apache.org/r/25356/diff/ Testing --- ./gradlew build -Pq Thanks, Bill Farner
Re: Review Request 25356: Add a factory to produce OneWayUpdate instances based on job state and update settings.
On Sept. 4, 2014, 10:17 p.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java, lines 94-96 https://reviews.apache.org/r/25356/diff/1/?file=679021#file679021line94 If I am reading it right the update will still proceed in case updateOnlyTheseInstances specifies an unrecognized range (i.e. not intersecting with the full working set). The InstanceUpdater appears to handle the optional desiredState as no-op. This is the departure from the current implementation where a disjoint --shards value fails the update. Bill Farner wrote: I think it's best for that to happen higher in the stack. The thrift interface is probably the best place for this, to validate a JobUpdateConfiguration when it's received. I am ok with that but it would require a bit of code duplication to rebuild the working set as we have to validate updateOnlyTheseInstances against the entire union of old and new instances. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25356/#review52363 --- On Sept. 5, 2014, 5:52 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25356/ --- (Updated Sept. 5, 2014, 5:52 p.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-613 https://issues.apache.org/jira/browse/AURORA-613 Repository: aurora Description --- This is the bridge between the top-level updater logic and the OneWayJobUpdater - taking a user request and the direction we've decided to move the job (forward or back), produce the appropriate OneWayJobUpdater. Diffs - src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java 85196af38b6615e5aac7de30a4a601350e935c72 src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java aa8b5f2ee81d42db003f27e682f79e94b6625d87 src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/updater/StateEvaluator.java dca55ef5612105c9dc8fd57e789fdf1df28ba447 src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java d7baaa679289998a9ea80c0934990a49e5c173d7 src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterTest.java e3e50d7d6526e8796dc5cd82b459190b09ceb72f Diff: https://reviews.apache.org/r/25356/diff/ Testing --- ./gradlew build -Pq Thanks, Bill Farner
Re: Review Request 25356: Add a factory to produce OneWayUpdate instances based on job state and update settings.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25356/#review52479 --- Ship it! Ship It! - Maxim Khutornenko On Sept. 5, 2014, 5:52 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25356/ --- (Updated Sept. 5, 2014, 5:52 p.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-613 https://issues.apache.org/jira/browse/AURORA-613 Repository: aurora Description --- This is the bridge between the top-level updater logic and the OneWayJobUpdater - taking a user request and the direction we've decided to move the job (forward or back), produce the appropriate OneWayJobUpdater. Diffs - src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java 85196af38b6615e5aac7de30a4a601350e935c72 src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java aa8b5f2ee81d42db003f27e682f79e94b6625d87 src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/updater/StateEvaluator.java dca55ef5612105c9dc8fd57e789fdf1df28ba447 src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java d7baaa679289998a9ea80c0934990a49e5c173d7 src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterTest.java e3e50d7d6526e8796dc5cd82b459190b09ceb72f Diff: https://reviews.apache.org/r/25356/diff/ Testing --- ./gradlew build -Pq Thanks, Bill Farner
Re: Review Request 25356: Add a factory to produce OneWayUpdate instances based on job state and update settings.
On Sept. 4, 2014, 10:17 p.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java, lines 94-96 https://reviews.apache.org/r/25356/diff/1/?file=679021#file679021line94 If I am reading it right the update will still proceed in case updateOnlyTheseInstances specifies an unrecognized range (i.e. not intersecting with the full working set). The InstanceUpdater appears to handle the optional desiredState as no-op. This is the departure from the current implementation where a disjoint --shards value fails the update. Bill Farner wrote: I think it's best for that to happen higher in the stack. The thrift interface is probably the best place for this, to validate a JobUpdateConfiguration when it's received. Maxim Khutornenko wrote: I am ok with that but it would require a bit of code duplication to rebuild the working set as we have to validate updateOnlyTheseInstances against the entire union of old and new instances. Aha, that's a very good point. I'll add it here instead. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25356/#review52363 --- On Sept. 5, 2014, 5:52 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25356/ --- (Updated Sept. 5, 2014, 5:52 p.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-613 https://issues.apache.org/jira/browse/AURORA-613 Repository: aurora Description --- This is the bridge between the top-level updater logic and the OneWayJobUpdater - taking a user request and the direction we've decided to move the job (forward or back), produce the appropriate OneWayJobUpdater. Diffs - src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java 85196af38b6615e5aac7de30a4a601350e935c72 src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java aa8b5f2ee81d42db003f27e682f79e94b6625d87 src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/updater/StateEvaluator.java dca55ef5612105c9dc8fd57e789fdf1df28ba447 src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java d7baaa679289998a9ea80c0934990a49e5c173d7 src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterTest.java e3e50d7d6526e8796dc5cd82b459190b09ceb72f Diff: https://reviews.apache.org/r/25356/diff/ Testing --- ./gradlew build -Pq Thanks, Bill Farner
Re: Review Request 25356: Add a factory to produce OneWayUpdate instances based on job state and update settings.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25356/ --- (Updated Sept. 5, 2014, 8:19 p.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-613 https://issues.apache.org/jira/browse/AURORA-613 Repository: aurora Description --- This is the bridge between the top-level updater logic and the OneWayJobUpdater - taking a user request and the direction we've decided to move the job (forward or back), produce the appropriate OneWayJobUpdater. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java 85196af38b6615e5aac7de30a4a601350e935c72 src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java aa8b5f2ee81d42db003f27e682f79e94b6625d87 src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/updater/StateEvaluator.java dca55ef5612105c9dc8fd57e789fdf1df28ba447 src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java d7baaa679289998a9ea80c0934990a49e5c173d7 src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterTest.java e3e50d7d6526e8796dc5cd82b459190b09ceb72f Diff: https://reviews.apache.org/r/25356/diff/ Testing --- ./gradlew build -Pq Thanks, Bill Farner
Review Request 25356: Add a factory to produce OneWayUpdate instances based on job state and update settings.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25356/ --- Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-613 https://issues.apache.org/jira/browse/AURORA-613 Repository: aurora Description --- This is the bridge between the top-level updater logic and the OneWayJobUpdater - taking a user request and the direction we've decided to move the job (forward or back), produce the appropriate OneWayJobUpdater. Diffs - src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java 85196af38b6615e5aac7de30a4a601350e935c72 src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java aa8b5f2ee81d42db003f27e682f79e94b6625d87 src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/updater/StateEvaluator.java dca55ef5612105c9dc8fd57e789fdf1df28ba447 src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java d7baaa679289998a9ea80c0934990a49e5c173d7 src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterTest.java e3e50d7d6526e8796dc5cd82b459190b09ceb72f Diff: https://reviews.apache.org/r/25356/diff/ Testing --- ./gradlew build -Pq Thanks, Bill Farner
Re: Review Request 25356: Add a factory to produce OneWayUpdate instances based on job state and update settings.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25356/#review52361 --- Ship it! Ship It! - Zameer Manji On Sept. 4, 2014, 2:19 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25356/ --- (Updated Sept. 4, 2014, 2:19 p.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-613 https://issues.apache.org/jira/browse/AURORA-613 Repository: aurora Description --- This is the bridge between the top-level updater logic and the OneWayJobUpdater - taking a user request and the direction we've decided to move the job (forward or back), produce the appropriate OneWayJobUpdater. Diffs - src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java 85196af38b6615e5aac7de30a4a601350e935c72 src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java aa8b5f2ee81d42db003f27e682f79e94b6625d87 src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/updater/StateEvaluator.java dca55ef5612105c9dc8fd57e789fdf1df28ba447 src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java d7baaa679289998a9ea80c0934990a49e5c173d7 src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterTest.java e3e50d7d6526e8796dc5cd82b459190b09ceb72f Diff: https://reviews.apache.org/r/25356/diff/ Testing --- ./gradlew build -Pq Thanks, Bill Farner
Re: Review Request 25356: Add a factory to produce OneWayUpdate instances based on job state and update settings.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25356/#review52363 --- src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java https://reviews.apache.org/r/25356/#comment91131 Does it have to be synchronized? Could not find the reason based on its call chain. src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java https://reviews.apache.org/r/25356/#comment91134 If I am reading it right the update will still proceed in case updateOnlyTheseInstances specifies an unrecognized range (i.e. not intersecting with the full working set). The InstanceUpdater appears to handle the optional desiredState as no-op. This is the departure from the current implementation where a disjoint --shards value fails the update. - Maxim Khutornenko On Sept. 4, 2014, 9:19 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25356/ --- (Updated Sept. 4, 2014, 9:19 p.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-613 https://issues.apache.org/jira/browse/AURORA-613 Repository: aurora Description --- This is the bridge between the top-level updater logic and the OneWayJobUpdater - taking a user request and the direction we've decided to move the job (forward or back), produce the appropriate OneWayJobUpdater. Diffs - src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java 85196af38b6615e5aac7de30a4a601350e935c72 src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java aa8b5f2ee81d42db003f27e682f79e94b6625d87 src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/updater/StateEvaluator.java dca55ef5612105c9dc8fd57e789fdf1df28ba447 src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java d7baaa679289998a9ea80c0934990a49e5c173d7 src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterTest.java e3e50d7d6526e8796dc5cd82b459190b09ceb72f Diff: https://reviews.apache.org/r/25356/diff/ Testing --- ./gradlew build -Pq Thanks, Bill Farner
