You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Bill Farner <wf...@apache.org> on 2014/09/04 23:19:51 UTC
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.
Posted by Bill Farner <wf...@apache.org>.
> 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.
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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.
Posted by Bill Farner <wf...@apache.org>.
> 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.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 25356: Add a factory to produce OneWayUpdate
instances based on job state and update settings.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
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
Re: Review Request 25356: Add a factory to produce OneWayUpdate
instances based on job state and update settings.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
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.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
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.
Posted by Zameer Manji <zm...@twopensource.com>.
-----------------------------------------------------------
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
>
>