You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@aurora.apache.org by Zameer Manji <zm...@gmail.com> on 2013/12/13 02:11:23 UTC
Review Request 16232: Add offer reservations to preemption flow
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/
-----------------------------------------------------------
Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
Bugs: AURORA-11
https://issues.apache.org/jira/browse/AURORA-11
Repository: aurora
Description
-------
This patch adds a reservation system the preemption flow.
The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
Diffs
-----
src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
Diff: https://reviews.apache.org/r/16232/diff/
Testing
-------
./gradlew clean build
Thanks,
Zameer Manji
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Zameer Manji <zm...@gmail.com>.
> On Dec. 13, 2013, 4:36 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java, lines 130-131
> > <https://reviews.apache.org/r/16232/diff/2/?file=397890#file397890line130>
> >
> > You already cNN on these in the Reservations constructor (good). I suggest removing this redundant check.
done.
> On Dec. 13, 2013, 4:36 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java, line 151
> > <https://reviews.apache.org/r/16232/diff/2/?file=397890#file397890line151>
> >
> > s/ ././
done.
> On Dec. 13, 2013, 4:36 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java, line 219
> > <https://reviews.apache.org/r/16232/diff/2/?file=397890#file397890line219>
> >
> > I'm still a bit weary of constructing SlaveID instances, since there are ways schema changes only fail at runtime. I suggest using String, and taking care to avoid ambiguity.
I think Map<String, String> is a huge code smell when one or both sides cannot be random strings. I used SlaveID to get the type system to prevent me from doing awful mistakes. Unless you strongly insist, I would like to continue to use SlaveID.
If I store a Map<String, String> I will still need to depend on the schema of SlaveID in the assigner function where I will be required to do offer.getSlaveId().getValue(), to look up the reservation. Either way I will need to depend on the schema of SlaveID and I think this way is better.
> On Dec. 13, 2013, 4:36 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java, line 224
> > <https://reviews.apache.org/r/16232/diff/2/?file=397890#file397890line224>
> >
> > This can come from a different thread, so you'll need to synchronize the methods in Reservations.
done.
> On Dec. 13, 2013, 4:36 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java, line 225
> > <https://reviews.apache.org/r/16232/diff/2/?file=397890#file397890line225>
> >
> > Low-hanging fruit for performance: check if the old state was PENDING. This way you do the O(n) map walk far less frequently.
this breaks my tests for some reason, punting on this.
> On Dec. 13, 2013, 4:36 p.m., Bill Farner wrote:
> > src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java, line 335
> > <https://reviews.apache.org/r/16232/diff/2/?file=397892#file397892line335>
> >
> > Use of Impl seems unnecessary here. Revert?
Not exactly. Before the interface was called SchedulingAction and the implementation was TaskScheduler. Now the interface is called TaskScheduler and the implementation is called TaskSchedulerImpl.
It previously was depending on the Impl and I am keeping it like that.
> On Dec. 13, 2013, 4:36 p.m., Bill Farner wrote:
> > src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java, lines 246-247
> > <https://reviews.apache.org/r/16232/diff/2/?file=397891#file397891line246>
> >
> > Looks like you'd benefit in test readability by introducing a helper function:
> >
> > private Capture<Function<Offer, Optional<TaskInfo>>> expectLaunchAttempt(boolean taskLaunched) {
> > ...
> > }
done.
- Zameer
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/#review30385
-----------------------------------------------------------
On Dec. 13, 2013, 4:16 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16232/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2013, 4:16 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
>
>
> Bugs: AURORA-11
> https://issues.apache.org/jira/browse/AURORA-11
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This patch adds a reservation system the preemption flow.
>
> The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
>
>
> Diffs
> -----
>
> src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
> src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
> src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
> src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
> src/test/java/com/twitter/aurora/scheduler/state/PubsubTestUtil.java f9d7fb64728008d0ea6eb424283b58e956e1d50a
>
> Diff: https://reviews.apache.org/r/16232/diff/
>
>
> Testing
> -------
>
> ./gradlew clean build
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Zameer Manji <zm...@twopensource.com>.
> On Dec. 13, 2013, 4:36 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java, line 225
> > <https://reviews.apache.org/r/16232/diff/2/?file=397890#file397890line225>
> >
> > Low-hanging fruit for performance: check if the old state was PENDING. This way you do the O(n) map walk far less frequently.
>
> Zameer Manji wrote:
> this breaks my tests for some reason, punting on this.
>
> Bill Farner wrote:
> Do you really think i'll accept that response? :-)
>
> Zameer Manji wrote:
> I meant punting to the next diff.
pushing this again to the next diff.
- Zameer
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/#review30385
-----------------------------------------------------------
On Dec. 13, 2013, 5:49 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16232/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2013, 5:49 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
>
>
> Bugs: AURORA-11
> https://issues.apache.org/jira/browse/AURORA-11
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This patch adds a reservation system the preemption flow.
>
> The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
>
>
> Diffs
> -----
>
> src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
> src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
> src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
> src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
> src/test/java/com/twitter/aurora/scheduler/state/PubsubTestUtil.java f9d7fb64728008d0ea6eb424283b58e956e1d50a
>
> Diff: https://reviews.apache.org/r/16232/diff/
>
>
> Testing
> -------
>
> ./gradlew clean build
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Zameer Manji <zm...@twopensource.com>.
> On Dec. 13, 2013, 4:36 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java, line 225
> > <https://reviews.apache.org/r/16232/diff/2/?file=397890#file397890line225>
> >
> > Low-hanging fruit for performance: check if the old state was PENDING. This way you do the O(n) map walk far less frequently.
>
> Zameer Manji wrote:
> this breaks my tests for some reason, punting on this.
>
> Bill Farner wrote:
> Do you really think i'll accept that response? :-)
>
> Zameer Manji wrote:
> I meant punting to the next diff.
>
> Zameer Manji wrote:
> pushing this again to the next diff.
done.
- Zameer
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/#review30385
-----------------------------------------------------------
On Dec. 16, 2013, 12:24 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16232/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2013, 12:24 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
>
>
> Bugs: AURORA-11
> https://issues.apache.org/jira/browse/AURORA-11
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This patch adds a reservation system the preemption flow.
>
> The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
>
>
> Diffs
> -----
>
> src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
> src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
> src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
> src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
> src/test/java/com/twitter/aurora/scheduler/state/PubsubTestUtil.java f9d7fb64728008d0ea6eb424283b58e956e1d50a
>
> Diff: https://reviews.apache.org/r/16232/diff/
>
>
> Testing
> -------
>
> ./gradlew clean build
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Zameer Manji <zm...@twopensource.com>.
> On Dec. 13, 2013, 4:36 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java, line 225
> > <https://reviews.apache.org/r/16232/diff/2/?file=397890#file397890line225>
> >
> > Low-hanging fruit for performance: check if the old state was PENDING. This way you do the O(n) map walk far less frequently.
>
> Zameer Manji wrote:
> this breaks my tests for some reason, punting on this.
>
> Bill Farner wrote:
> Do you really think i'll accept that response? :-)
I meant punting to the next diff.
- Zameer
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/#review30385
-----------------------------------------------------------
On Dec. 13, 2013, 5:49 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16232/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2013, 5:49 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
>
>
> Bugs: AURORA-11
> https://issues.apache.org/jira/browse/AURORA-11
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This patch adds a reservation system the preemption flow.
>
> The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
>
>
> Diffs
> -----
>
> src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
> src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
> src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
> src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
> src/test/java/com/twitter/aurora/scheduler/state/PubsubTestUtil.java f9d7fb64728008d0ea6eb424283b58e956e1d50a
>
> Diff: https://reviews.apache.org/r/16232/diff/
>
>
> Testing
> -------
>
> ./gradlew clean build
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Bill Farner <wf...@apache.org>.
> On Dec. 14, 2013, 12:36 a.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java, line 225
> > <https://reviews.apache.org/r/16232/diff/2/?file=397890#file397890line225>
> >
> > Low-hanging fruit for performance: check if the old state was PENDING. This way you do the O(n) map walk far less frequently.
>
> Zameer Manji wrote:
> this breaks my tests for some reason, punting on this.
Do you really think i'll accept that response? :-)
> On Dec. 14, 2013, 12:36 a.m., Bill Farner wrote:
> > src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java, line 335
> > <https://reviews.apache.org/r/16232/diff/2/?file=397892#file397892line335>
> >
> > Use of Impl seems unnecessary here. Revert?
>
> Zameer Manji wrote:
> Not exactly. Before the interface was called SchedulingAction and the implementation was TaskScheduler. Now the interface is called TaskScheduler and the implementation is called TaskSchedulerImpl.
>
> It previously was depending on the Impl and I am keeping it like that.
Ah, gotcha — thanks for clarifying.
> On Dec. 14, 2013, 12:36 a.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java, line 219
> > <https://reviews.apache.org/r/16232/diff/2/?file=397890#file397890line219>
> >
> > I'm still a bit weary of constructing SlaveID instances, since there are ways schema changes only fail at runtime. I suggest using String, and taking care to avoid ambiguity.
>
> Zameer Manji wrote:
> I think Map<String, String> is a huge code smell when one or both sides cannot be random strings. I used SlaveID to get the type system to prevent me from doing awful mistakes. Unless you strongly insist, I would like to continue to use SlaveID.
>
> If I store a Map<String, String> I will still need to depend on the schema of SlaveID in the assigner function where I will be required to do offer.getSlaveId().getValue(), to look up the reservation. Either way I will need to depend on the schema of SlaveID and I think this way is better.
Ok, i'll back down — in practice, schema changes should trip in unit tests anyway.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/#review30385
-----------------------------------------------------------
On Dec. 14, 2013, 1:49 a.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16232/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2013, 1:49 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
>
>
> Bugs: AURORA-11
> https://issues.apache.org/jira/browse/AURORA-11
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This patch adds a reservation system the preemption flow.
>
> The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
>
>
> Diffs
> -----
>
> src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
> src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
> src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
> src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
> src/test/java/com/twitter/aurora/scheduler/state/PubsubTestUtil.java f9d7fb64728008d0ea6eb424283b58e956e1d50a
>
> Diff: https://reviews.apache.org/r/16232/diff/
>
>
> Testing
> -------
>
> ./gradlew clean build
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/#review30385
-----------------------------------------------------------
src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java
<https://reviews.apache.org/r/16232/#comment58166>
You already cNN on these in the Reservations constructor (good). I suggest removing this redundant check.
src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java
<https://reviews.apache.org/r/16232/#comment58167>
s/ ././
src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java
<https://reviews.apache.org/r/16232/#comment58161>
I'm still a bit weary of constructing SlaveID instances, since there are ways schema changes only fail at runtime. I suggest using String, and taking care to avoid ambiguity.
src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java
<https://reviews.apache.org/r/16232/#comment58160>
This can come from a different thread, so you'll need to synchronize the methods in Reservations.
src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java
<https://reviews.apache.org/r/16232/#comment58162>
Low-hanging fruit for performance: check if the old state was PENDING. This way you do the O(n) map walk far less frequently.
src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java
<https://reviews.apache.org/r/16232/#comment58165>
Looks like you'd benefit in test readability by introducing a helper function:
private Capture<Function<Offer, Optional<TaskInfo>>> expectLaunchAttempt(boolean taskLaunched) {
...
}
src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java
<https://reviews.apache.org/r/16232/#comment58164>
Use of Impl seems unnecessary here. Revert?
- Bill Farner
On Dec. 14, 2013, 12:16 a.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16232/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2013, 12:16 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
>
>
> Bugs: AURORA-11
> https://issues.apache.org/jira/browse/AURORA-11
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This patch adds a reservation system the preemption flow.
>
> The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
>
>
> Diffs
> -----
>
> src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
> src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
> src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
> src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
> src/test/java/com/twitter/aurora/scheduler/state/PubsubTestUtil.java f9d7fb64728008d0ea6eb424283b58e956e1d50a
>
> Diff: https://reviews.apache.org/r/16232/diff/
>
>
> Testing
> -------
>
> ./gradlew clean build
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/#review30589
-----------------------------------------------------------
Ship it!
Thanks for biting the bullet on the testing change, this looks great!
src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java
<https://reviews.apache.org/r/16232/#comment58590>
It would be good (and more readable) to validate the returned value from schedule(), mind doing that here and elsewhere?
- Bill Farner
On Dec. 17, 2013, 11:19 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16232/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2013, 11:19 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
>
>
> Bugs: AURORA-11
> https://issues.apache.org/jira/browse/AURORA-11
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This patch adds a reservation system the preemption flow.
>
> The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
>
>
> Diffs
> -----
>
> src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
> src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
> src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
> src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
> src/test/java/com/twitter/aurora/scheduler/state/PubsubTestUtil.java f9d7fb64728008d0ea6eb424283b58e956e1d50a
>
> Diff: https://reviews.apache.org/r/16232/diff/
>
>
> Testing
> -------
>
> ./gradlew clean build
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/#review30633
-----------------------------------------------------------
This is now on master, please close at your earliest convenience.
- Bill Farner
On Dec. 18, 2013, 12:50 a.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16232/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2013, 12:50 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
>
>
> Bugs: AURORA-11
> https://issues.apache.org/jira/browse/AURORA-11
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This patch adds a reservation system the preemption flow.
>
> The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
>
>
> Diffs
> -----
>
> src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
> src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
> src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
> src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
> src/test/java/com/twitter/aurora/scheduler/state/PubsubTestUtil.java f9d7fb64728008d0ea6eb424283b58e956e1d50a
>
> Diff: https://reviews.apache.org/r/16232/diff/
>
>
> Testing
> -------
>
> ./gradlew clean build
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Zameer Manji <zm...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/
-----------------------------------------------------------
(Updated Dec. 17, 2013, 4:50 p.m.)
Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
Changes
-------
Assert return values from schedule method.
Bugs: AURORA-11
https://issues.apache.org/jira/browse/AURORA-11
Repository: aurora
Description
-------
This patch adds a reservation system the preemption flow.
The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
Diffs (updated)
-----
src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
src/test/java/com/twitter/aurora/scheduler/state/PubsubTestUtil.java f9d7fb64728008d0ea6eb424283b58e956e1d50a
Diff: https://reviews.apache.org/r/16232/diff/
Testing
-------
./gradlew clean build
Thanks,
Zameer Manji
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Zameer Manji <zm...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/
-----------------------------------------------------------
(Updated Dec. 17, 2013, 3:19 p.m.)
Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
Changes
-------
Improve tests to not use preempt()
Bugs: AURORA-11
https://issues.apache.org/jira/browse/AURORA-11
Repository: aurora
Description
-------
This patch adds a reservation system the preemption flow.
The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
Diffs (updated)
-----
src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
src/test/java/com/twitter/aurora/scheduler/state/PubsubTestUtil.java f9d7fb64728008d0ea6eb424283b58e956e1d50a
Diff: https://reviews.apache.org/r/16232/diff/
Testing
-------
./gradlew clean build
Thanks,
Zameer Manji
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Zameer Manji <zm...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/
-----------------------------------------------------------
(Updated Dec. 16, 2013, 4:09 p.m.)
Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
Changes
-------
Addressed Bill's Feedback.
Bugs: AURORA-11
https://issues.apache.org/jira/browse/AURORA-11
Repository: aurora
Description
-------
This patch adds a reservation system the preemption flow.
The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
Diffs (updated)
-----
src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
src/test/java/com/twitter/aurora/scheduler/state/PubsubTestUtil.java f9d7fb64728008d0ea6eb424283b58e956e1d50a
Diff: https://reviews.apache.org/r/16232/diff/
Testing
-------
./gradlew clean build
Thanks,
Zameer Manji
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Bill Farner <wf...@apache.org>.
> On Dec. 16, 2013, 11:23 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java, line 210
> > <https://reviews.apache.org/r/16232/diff/5/?file=398504#file398504line210>
> >
> > Since the plumbing seems to be in place to avoid exposing this, can you try to make it private? This has the added benefit of sidestepping exposing TaskSchedulerImpl in AsyncModule.
>
> Zameer Manji wrote:
> It just makes the tests too hard to read IMHO. Even creating test helper methods just make it difficult to understand what is expected and what should not be happening. I also feel a serious attempt should be in another diff.
>
> Bill Farner wrote:
> I'm generally non-optimistic over the promise of better testing later. I would very much prefer to sort this out upfront to fix the warts mentioned above.
Let's pair offline and investigate how the tests wind up looking if preempt() is not used directly.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/#review30498
-----------------------------------------------------------
On Dec. 17, 2013, 12:09 a.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16232/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2013, 12:09 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
>
>
> Bugs: AURORA-11
> https://issues.apache.org/jira/browse/AURORA-11
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This patch adds a reservation system the preemption flow.
>
> The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
>
>
> Diffs
> -----
>
> src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
> src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
> src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
> src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
> src/test/java/com/twitter/aurora/scheduler/state/PubsubTestUtil.java f9d7fb64728008d0ea6eb424283b58e956e1d50a
>
> Diff: https://reviews.apache.org/r/16232/diff/
>
>
> Testing
> -------
>
> ./gradlew clean build
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Zameer Manji <zm...@twopensource.com>.
> On Dec. 16, 2013, 3:23 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java, line 210
> > <https://reviews.apache.org/r/16232/diff/5/?file=398504#file398504line210>
> >
> > Since the plumbing seems to be in place to avoid exposing this, can you try to make it private? This has the added benefit of sidestepping exposing TaskSchedulerImpl in AsyncModule.
>
> Zameer Manji wrote:
> It just makes the tests too hard to read IMHO. Even creating test helper methods just make it difficult to understand what is expected and what should not be happening. I also feel a serious attempt should be in another diff.
>
> Bill Farner wrote:
> I'm generally non-optimistic over the promise of better testing later. I would very much prefer to sort this out upfront to fix the warts mentioned above.
>
> Bill Farner wrote:
> Let's pair offline and investigate how the tests wind up looking if preempt() is not used directly.
done.
- Zameer
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/#review30498
-----------------------------------------------------------
On Dec. 17, 2013, 3:19 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16232/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2013, 3:19 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
>
>
> Bugs: AURORA-11
> https://issues.apache.org/jira/browse/AURORA-11
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This patch adds a reservation system the preemption flow.
>
> The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
>
>
> Diffs
> -----
>
> src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
> src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
> src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
> src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
> src/test/java/com/twitter/aurora/scheduler/state/PubsubTestUtil.java f9d7fb64728008d0ea6eb424283b58e956e1d50a
>
> Diff: https://reviews.apache.org/r/16232/diff/
>
>
> Testing
> -------
>
> ./gradlew clean build
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Bill Farner <wf...@apache.org>.
> On Dec. 16, 2013, 11:23 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java, line 210
> > <https://reviews.apache.org/r/16232/diff/5/?file=398504#file398504line210>
> >
> > Since the plumbing seems to be in place to avoid exposing this, can you try to make it private? This has the added benefit of sidestepping exposing TaskSchedulerImpl in AsyncModule.
>
> Zameer Manji wrote:
> It just makes the tests too hard to read IMHO. Even creating test helper methods just make it difficult to understand what is expected and what should not be happening. I also feel a serious attempt should be in another diff.
I'm generally non-optimistic over the promise of better testing later. I would very much prefer to sort this out upfront to fix the warts mentioned above.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/#review30498
-----------------------------------------------------------
On Dec. 17, 2013, 12:09 a.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16232/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2013, 12:09 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
>
>
> Bugs: AURORA-11
> https://issues.apache.org/jira/browse/AURORA-11
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This patch adds a reservation system the preemption flow.
>
> The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
>
>
> Diffs
> -----
>
> src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
> src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
> src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
> src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
> src/test/java/com/twitter/aurora/scheduler/state/PubsubTestUtil.java f9d7fb64728008d0ea6eb424283b58e956e1d50a
>
> Diff: https://reviews.apache.org/r/16232/diff/
>
>
> Testing
> -------
>
> ./gradlew clean build
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Bill Farner <wf...@apache.org>.
> On Dec. 16, 2013, 11:23 p.m., Bill Farner wrote:
> >
This is looking really good, just a few things remaining.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/#review30498
-----------------------------------------------------------
On Dec. 16, 2013, 11:04 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16232/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2013, 11:04 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
>
>
> Bugs: AURORA-11
> https://issues.apache.org/jira/browse/AURORA-11
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This patch adds a reservation system the preemption flow.
>
> The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
>
>
> Diffs
> -----
>
> src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
> src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
> src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
> src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
> src/test/java/com/twitter/aurora/scheduler/state/PubsubTestUtil.java f9d7fb64728008d0ea6eb424283b58e956e1d50a
>
> Diff: https://reviews.apache.org/r/16232/diff/
>
>
> Testing
> -------
>
> ./gradlew clean build
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Zameer Manji <zm...@twopensource.com>.
> On Dec. 16, 2013, 3:23 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java, line 181
> > <https://reviews.apache.org/r/16232/diff/5/?file=398501#file398501line181>
> >
> > revert
done.
> On Dec. 16, 2013, 3:23 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java, line 247
> > <https://reviews.apache.org/r/16232/diff/5/?file=398501#file398501line247>
> >
> > remove extra newline
done.
> On Dec. 16, 2013, 3:23 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java, line 250
> > <https://reviews.apache.org/r/16232/diff/5/?file=398501#file398501line250>
> >
> > when you said "i'll fix formatting later" in person, i had a hunch it wouldn't happen :-)
done. One day I am going to get IntelliJ to do this for me.
> On Dec. 16, 2013, 3:23 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java, line 256
> > <https://reviews.apache.org/r/16232/diff/5/?file=398501#file398501line256>
> >
> > put the line break before .toinstance rather than here
done.
> On Dec. 16, 2013, 3:23 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java, line 259
> > <https://reviews.apache.org/r/16232/diff/5/?file=398501#file398501line259>
> >
> > remove this comment, it's the other one that stands out as strange
done.
> On Dec. 16, 2013, 3:23 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java, line 176
> > <https://reviews.apache.org/r/16232/diff/5/?file=398503#file398503line176>
> >
> > Style nit: i prefer to leave an empty line between switch case blocks:
> >
> > switch (result) {
> > case SUCCESS:
> > ...
> > break;
> >
> > case TRY_LATER:
> > ...
done.
> On Dec. 16, 2013, 3:23 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java, line 82
> > <https://reviews.apache.org/r/16232/diff/5/?file=398504#file398504line82>
> >
> > s/public //. everything declared in an interface is implicitly public
done.
> On Dec. 16, 2013, 3:23 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java, line 181
> > <https://reviews.apache.org/r/16232/diff/5/?file=398504#file398504line181>
> >
> > How about "maybePreemptFor"?
done.
> On Dec. 16, 2013, 3:23 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java, line 210
> > <https://reviews.apache.org/r/16232/diff/5/?file=398504#file398504line210>
> >
> > Since the plumbing seems to be in place to avoid exposing this, can you try to make it private? This has the added benefit of sidestepping exposing TaskSchedulerImpl in AsyncModule.
It just makes the tests too hard to read IMHO. Even creating test helper methods just make it difficult to understand what is expected and what should not be happening. I also feel a serious attempt should be in another diff.
> On Dec. 16, 2013, 3:23 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java, line 249
> > <https://reviews.apache.org/r/16232/diff/5/?file=398504#file398504line249>
> >
> > s/this.//
done.
- Zameer
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/#review30498
-----------------------------------------------------------
On Dec. 16, 2013, 3:04 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16232/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2013, 3:04 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
>
>
> Bugs: AURORA-11
> https://issues.apache.org/jira/browse/AURORA-11
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This patch adds a reservation system the preemption flow.
>
> The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
>
>
> Diffs
> -----
>
> src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
> src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
> src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
> src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
> src/test/java/com/twitter/aurora/scheduler/state/PubsubTestUtil.java f9d7fb64728008d0ea6eb424283b58e956e1d50a
>
> Diff: https://reviews.apache.org/r/16232/diff/
>
>
> Testing
> -------
>
> ./gradlew clean build
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/#review30498
-----------------------------------------------------------
src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java
<https://reviews.apache.org/r/16232/#comment58344>
revert
src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java
<https://reviews.apache.org/r/16232/#comment58345>
remove extra newline
src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java
<https://reviews.apache.org/r/16232/#comment58347>
when you said "i'll fix formatting later" in person, i had a hunch it wouldn't happen :-)
src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java
<https://reviews.apache.org/r/16232/#comment58348>
put the line break before .toinstance rather than here
src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java
<https://reviews.apache.org/r/16232/#comment58349>
remove this comment, it's the other one that stands out as strange
src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java
<https://reviews.apache.org/r/16232/#comment58350>
Style nit: i prefer to leave an empty line between switch case blocks:
switch (result) {
case SUCCESS:
...
break;
case TRY_LATER:
...
src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java
<https://reviews.apache.org/r/16232/#comment58351>
s/public //. everything declared in an interface is implicitly public
src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java
<https://reviews.apache.org/r/16232/#comment58353>
How about "maybePreemptFor"?
src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java
<https://reviews.apache.org/r/16232/#comment58352>
Since the plumbing seems to be in place to avoid exposing this, can you try to make it private? This has the added benefit of sidestepping exposing TaskSchedulerImpl in AsyncModule.
src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java
<https://reviews.apache.org/r/16232/#comment58354>
s/this.//
- Bill Farner
On Dec. 16, 2013, 11:04 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16232/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2013, 11:04 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
>
>
> Bugs: AURORA-11
> https://issues.apache.org/jira/browse/AURORA-11
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This patch adds a reservation system the preemption flow.
>
> The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
>
>
> Diffs
> -----
>
> src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
> src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
> src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
> src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
> src/test/java/com/twitter/aurora/scheduler/state/PubsubTestUtil.java f9d7fb64728008d0ea6eb424283b58e956e1d50a
>
> Diff: https://reviews.apache.org/r/16232/diff/
>
>
> Testing
> -------
>
> ./gradlew clean build
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Zameer Manji <zm...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/
-----------------------------------------------------------
(Updated Dec. 16, 2013, 3:04 p.m.)
Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
Changes
-------
Addressed all of the feedback.
Bugs: AURORA-11
https://issues.apache.org/jira/browse/AURORA-11
Repository: aurora
Description
-------
This patch adds a reservation system the preemption flow.
The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
Diffs (updated)
-----
src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
src/test/java/com/twitter/aurora/scheduler/state/PubsubTestUtil.java f9d7fb64728008d0ea6eb424283b58e956e1d50a
Diff: https://reviews.apache.org/r/16232/diff/
Testing
-------
./gradlew clean build
Thanks,
Zameer Manji
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Zameer Manji <zm...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/
-----------------------------------------------------------
(Updated Dec. 16, 2013, 12:24 p.m.)
Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
Changes
-------
Acted on more feedback.
Bugs: AURORA-11
https://issues.apache.org/jira/browse/AURORA-11
Repository: aurora
Description
-------
This patch adds a reservation system the preemption flow.
The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
Diffs (updated)
-----
src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
src/test/java/com/twitter/aurora/scheduler/state/PubsubTestUtil.java f9d7fb64728008d0ea6eb424283b58e956e1d50a
Diff: https://reviews.apache.org/r/16232/diff/
Testing
-------
./gradlew clean build
Thanks,
Zameer Manji
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Zameer Manji <zm...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/
-----------------------------------------------------------
(Updated Dec. 13, 2013, 5:49 p.m.)
Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
Changes
-------
Acted on Bill's feedback.
Bugs: AURORA-11
https://issues.apache.org/jira/browse/AURORA-11
Repository: aurora
Description
-------
This patch adds a reservation system the preemption flow.
The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
Diffs (updated)
-----
src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
src/test/java/com/twitter/aurora/scheduler/state/PubsubTestUtil.java f9d7fb64728008d0ea6eb424283b58e956e1d50a
Diff: https://reviews.apache.org/r/16232/diff/
Testing
-------
./gradlew clean build
Thanks,
Zameer Manji
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Zameer Manji <zm...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/
-----------------------------------------------------------
(Updated Dec. 13, 2013, 4:16 p.m.)
Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
Changes
-------
This addresses most of the feedback from Bill.
Bugs: AURORA-11
https://issues.apache.org/jira/browse/AURORA-11
Repository: aurora
Description
-------
This patch adds a reservation system the preemption flow.
The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
Diffs (updated)
-----
src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
src/test/java/com/twitter/aurora/scheduler/state/PubsubTestUtil.java f9d7fb64728008d0ea6eb424283b58e956e1d50a
Diff: https://reviews.apache.org/r/16232/diff/
Testing
-------
./gradlew clean build
Thanks,
Zameer Manji
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Zameer Manji <zm...@twopensource.com>.
> On Dec. 12, 2013, 5:35 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java, line 193
> > <https://reviews.apache.org/r/16232/diff/1/?file=397098#file397098line193>
> >
> > I'm 99% this works fine because you've exposed the key you're binding against (i suspect that's _why_ you exposed it), but can you confirm that a test fails if this is not wired properly?
>
> Zameer Manji wrote:
> I ran into some problems doing this, lets talk offline. The current diff now fails SchedulerIT.testLaunch
Still failing on this diff, FYI.
> On Dec. 12, 2013, 5:35 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java, line 182
> > <https://reviews.apache.org/r/16232/diff/1/?file=397100#file397100line182>
> >
> > Realizing this may have been dropped in the internal review:
> >
> > Thinking even further on this, perhaps the tryPreempt() behavior should be implicit to schedule(), and the schedule() outcome becomes an enum SUCCESS, TRY_LATER?
>
> Zameer Manji wrote:
> I would like to discuss this offline before I do it.
done.
- Zameer
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/#review30317
-----------------------------------------------------------
On Dec. 13, 2013, 5:49 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16232/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2013, 5:49 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
>
>
> Bugs: AURORA-11
> https://issues.apache.org/jira/browse/AURORA-11
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This patch adds a reservation system the preemption flow.
>
> The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
>
>
> Diffs
> -----
>
> src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
> src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
> src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
> src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
> src/test/java/com/twitter/aurora/scheduler/state/PubsubTestUtil.java f9d7fb64728008d0ea6eb424283b58e956e1d50a
>
> Diff: https://reviews.apache.org/r/16232/diff/
>
>
> Testing
> -------
>
> ./gradlew clean build
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Zameer Manji <zm...@gmail.com>.
> On Dec. 12, 2013, 5:35 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java, line 128
> > <https://reviews.apache.org/r/16232/diff/1/?file=397098#file397098line128>
> >
> > The help string could use some more detail, thinking out loud:
> >
> > "Time to reserve a slave's offers while trying to satisfy a task preempting another."
done.
> On Dec. 12, 2013, 5:35 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java, line 193
> > <https://reviews.apache.org/r/16232/diff/1/?file=397098#file397098line193>
> >
> > I'm 99% this works fine because you've exposed the key you're binding against (i suspect that's _why_ you exposed it), but can you confirm that a test fails if this is not wired properly?
I ran into some problems doing this, lets talk offline. The current diff now fails SchedulerIT.testLaunch
> On Dec. 12, 2013, 5:35 p.m., Bill Farner wrote:
> > src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java, line 229
> > <https://reviews.apache.org/r/16232/diff/1/?file=397102#file397102line229>
> >
> > extra newline
done.
> On Dec. 12, 2013, 5:35 p.m., Bill Farner wrote:
> > src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java, line 99
> > <https://reviews.apache.org/r/16232/diff/1/?file=397102#file397102line99>
> >
> > I should have given better guidance in the last round. What i meant was just let them propagate unchanged, by removing try/catch, and changing the test method signature to "throws Exception".
done
> On Dec. 12, 2013, 5:35 p.m., Bill Farner wrote:
> > src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java, line 139
> > <https://reviews.apache.org/r/16232/diff/1/?file=397102#file397102line139>
> >
> > please remove all of these
done.
> On Dec. 12, 2013, 5:35 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java, line 268
> > <https://reviews.apache.org/r/16232/diff/1/?file=397101#file397101line268>
> >
> > This routine can be simplified, since Cache's contract states that asMap() returns a view, and modifications are possible through the view. So, you can do this:
> >
> > reservations.asMap().values().remove(taskId);
Done, it really simplifies the code.
> On Dec. 12, 2013, 5:35 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java, line 141
> > <https://reviews.apache.org/r/16232/diff/1/?file=397101#file397101line141>
> >
> > please leave an empty line between wrapped method signature and body
done.
> On Dec. 12, 2013, 5:35 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java, line 143
> > <https://reviews.apache.org/r/16232/diff/1/?file=397101#file397101line143>
> >
> > How about getSlaveReservation()? "is" implies boolean to me, while "get" suggests a response (which might be empty)
done.
> On Dec. 12, 2013, 5:35 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java, line 72
> > <https://reviews.apache.org/r/16232/diff/1/?file=397101#file397101line72>
> >
> > Better doc?
done.
> On Dec. 12, 2013, 5:35 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java, line 144
> > <https://reviews.apache.org/r/16232/diff/1/?file=397101#file397101line144>
> >
> > Some tiny comments would improve readability of this tri-state behavior:
> >
> > if (reservedTaskId.isPresent()) {
> > if (taskId.equals(reservedTaskId.get()) {
> > // Slave is reserved to satisfy this task.
> > return assigner.maybeAssign(offer, task);
> > } else {
> > // Slave is reserved for another task.
> > return Optional.absent();
> > }
> > } else {
> > // Slave is not reserved.
> > return assigner.maybeAssign(offer, task);
> > }
done.
> On Dec. 12, 2013, 5:35 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java, line 182
> > <https://reviews.apache.org/r/16232/diff/1/?file=397100#file397100line182>
> >
> > Realizing this may have been dropped in the internal review:
> >
> > Thinking even further on this, perhaps the tryPreempt() behavior should be implicit to schedule(), and the schedule() outcome becomes an enum SUCCESS, TRY_LATER?
I would like to discuss this offline before I do it.
> On Dec. 12, 2013, 5:35 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java, line 146
> > <https://reviews.apache.org/r/16232/diff/1/?file=397101#file397101line146>
> >
> > There's a bit of a feature gap here — if the task _is_ assigned, the reservation should be cleared so we can promptly schedule others. It may be tempting to call the O(n) invalidateTask(), but i suggest adding the O(1) invalidateSlave().
> >
> > Please write a failing test for this behavior, then fix.
Pushing this to the next diff.
- Zameer
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/#review30317
-----------------------------------------------------------
On Dec. 12, 2013, 5:11 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16232/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2013, 5:11 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
>
>
> Bugs: AURORA-11
> https://issues.apache.org/jira/browse/AURORA-11
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This patch adds a reservation system the preemption flow.
>
> The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
>
>
> Diffs
> -----
>
> src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
> src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
> src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
> src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
>
> Diff: https://reviews.apache.org/r/16232/diff/
>
>
> Testing
> -------
>
> ./gradlew clean build
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Zameer Manji <zm...@gmail.com>.
> On Dec. 12, 2013, 5:35 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java, line 146
> > <https://reviews.apache.org/r/16232/diff/1/?file=397101#file397101line146>
> >
> > There's a bit of a feature gap here — if the task _is_ assigned, the reservation should be cleared so we can promptly schedule others. It may be tempting to call the O(n) invalidateTask(), but i suggest adding the O(1) invalidateSlave().
> >
> > Please write a failing test for this behavior, then fix.
>
> Zameer Manji wrote:
> Pushing this to the next diff.
Actually not doing this just yet. This complicates the code because now it removes the reservation either when the taskState changes or when we assign the task. I strongly prefer keeping it simple and letting reservation removal occur via pubsub.
- Zameer
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/#review30317
-----------------------------------------------------------
On Dec. 13, 2013, 4:16 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16232/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2013, 4:16 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
>
>
> Bugs: AURORA-11
> https://issues.apache.org/jira/browse/AURORA-11
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This patch adds a reservation system the preemption flow.
>
> The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
>
>
> Diffs
> -----
>
> src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
> src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
> src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
> src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
> src/test/java/com/twitter/aurora/scheduler/state/PubsubTestUtil.java f9d7fb64728008d0ea6eb424283b58e956e1d50a
>
> Diff: https://reviews.apache.org/r/16232/diff/
>
>
> Testing
> -------
>
> ./gradlew clean build
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Bill Farner <wf...@apache.org>.
> On Dec. 13, 2013, 1:35 a.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java, line 146
> > <https://reviews.apache.org/r/16232/diff/1/?file=397101#file397101line146>
> >
> > There's a bit of a feature gap here — if the task _is_ assigned, the reservation should be cleared so we can promptly schedule others. It may be tempting to call the O(n) invalidateTask(), but i suggest adding the O(1) invalidateSlave().
> >
> > Please write a failing test for this behavior, then fix.
>
> Zameer Manji wrote:
> Pushing this to the next diff.
>
> Zameer Manji wrote:
> Actually not doing this just yet. This complicates the code because now it removes the reservation either when the taskState changes or when we assign the task. I strongly prefer keeping it simple and letting reservation removal occur via pubsub.
Oh, duh — braindead response on my part. I forgot that the assignment would induce a state change. This LGTM.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/#review30317
-----------------------------------------------------------
On Dec. 14, 2013, 12:16 a.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16232/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2013, 12:16 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
>
>
> Bugs: AURORA-11
> https://issues.apache.org/jira/browse/AURORA-11
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This patch adds a reservation system the preemption flow.
>
> The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
>
>
> Diffs
> -----
>
> src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
> src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
> src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
> src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
> src/test/java/com/twitter/aurora/scheduler/state/PubsubTestUtil.java f9d7fb64728008d0ea6eb424283b58e956e1d50a
>
> Diff: https://reviews.apache.org/r/16232/diff/
>
>
> Testing
> -------
>
> ./gradlew clean build
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Zameer Manji <zm...@twopensource.com>.
> On Dec. 12, 2013, 5:35 p.m., Bill Farner wrote:
> > src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java, line 193
> > <https://reviews.apache.org/r/16232/diff/1/?file=397098#file397098line193>
> >
> > I'm 99% this works fine because you've exposed the key you're binding against (i suspect that's _why_ you exposed it), but can you confirm that a test fails if this is not wired properly?
>
> Zameer Manji wrote:
> I ran into some problems doing this, lets talk offline. The current diff now fails SchedulerIT.testLaunch
>
> Zameer Manji wrote:
> Still failing on this diff, FYI.
done.
- Zameer
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/#review30317
-----------------------------------------------------------
On Dec. 16, 2013, 12:24 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16232/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2013, 12:24 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
>
>
> Bugs: AURORA-11
> https://issues.apache.org/jira/browse/AURORA-11
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This patch adds a reservation system the preemption flow.
>
> The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
>
>
> Diffs
> -----
>
> src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
> src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
> src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
> src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
> src/test/java/com/twitter/aurora/scheduler/state/PubsubTestUtil.java f9d7fb64728008d0ea6eb424283b58e956e1d50a
>
> Diff: https://reviews.apache.org/r/16232/diff/
>
>
> Testing
> -------
>
> ./gradlew clean build
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 16232: Add offer reservations to preemption flow
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16232/#review30317
-----------------------------------------------------------
src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java
<https://reviews.apache.org/r/16232/#comment58034>
The help string could use some more detail, thinking out loud:
"Time to reserve a slave's offers while trying to satisfy a task preempting another."
src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java
<https://reviews.apache.org/r/16232/#comment58035>
I'm 99% this works fine because you've exposed the key you're binding against (i suspect that's _why_ you exposed it), but can you confirm that a test fails if this is not wired properly?
src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java
<https://reviews.apache.org/r/16232/#comment58036>
Realizing this may have been dropped in the internal review:
Thinking even further on this, perhaps the tryPreempt() behavior should be implicit to schedule(), and the schedule() outcome becomes an enum SUCCESS, TRY_LATER?
src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java
<https://reviews.apache.org/r/16232/#comment58037>
Better doc?
src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java
<https://reviews.apache.org/r/16232/#comment58038>
please leave an empty line between wrapped method signature and body
src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java
<https://reviews.apache.org/r/16232/#comment58039>
How about getSlaveReservation()? "is" implies boolean to me, while "get" suggests a response (which might be empty)
src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java
<https://reviews.apache.org/r/16232/#comment58043>
Some tiny comments would improve readability of this tri-state behavior:
if (reservedTaskId.isPresent()) {
if (taskId.equals(reservedTaskId.get()) {
// Slave is reserved to satisfy this task.
return assigner.maybeAssign(offer, task);
} else {
// Slave is reserved for another task.
return Optional.absent();
}
} else {
// Slave is not reserved.
return assigner.maybeAssign(offer, task);
}
src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java
<https://reviews.apache.org/r/16232/#comment58045>
There's a bit of a feature gap here — if the task _is_ assigned, the reservation should be cleared so we can promptly schedule others. It may be tempting to call the O(n) invalidateTask(), but i suggest adding the O(1) invalidateSlave().
Please write a failing test for this behavior, then fix.
src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java
<https://reviews.apache.org/r/16232/#comment58049>
This routine can be simplified, since Cache's contract states that asMap() returns a view, and modifications are possible through the view. So, you can do this:
reservations.asMap().values().remove(taskId);
src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java
<https://reviews.apache.org/r/16232/#comment58051>
I should have given better guidance in the last round. What i meant was just let them propagate unchanged, by removing try/catch, and changing the test method signature to "throws Exception".
src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java
<https://reviews.apache.org/r/16232/#comment58052>
please remove all of these
src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java
<https://reviews.apache.org/r/16232/#comment58053>
extra newline
- Bill Farner
On Dec. 13, 2013, 1:11 a.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16232/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2013, 1:11 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
>
>
> Bugs: AURORA-11
> https://issues.apache.org/jira/browse/AURORA-11
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This patch adds a reservation system the preemption flow.
>
> The reservation associates a slave id with a task id for a fixed duration. If the task attempts to schedule itself during that time period and an offer is available from that slave then it will be scheduled. If another task attempts to schedule itself then it will not use the reserved offer.
>
>
> Diffs
> -----
>
> src/main/java/com/twitter/aurora/scheduler/async/AsyncModule.java db07841543e554e269f6fe7b36d7f7232af21140
> src/main/java/com/twitter/aurora/scheduler/async/Preemptor.java e5aeb8321e22c51eb3a5dad3d3dd1e26b7121b7d
> src/main/java/com/twitter/aurora/scheduler/async/TaskGroups.java f95f719c5a444b0f8faa4330852e251dd5de528e
> src/main/java/com/twitter/aurora/scheduler/async/TaskScheduler.java fbd82ff70235294cfd27c242f141a585d6bb2396
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerImplTest.java PRE-CREATION
> src/test/java/com/twitter/aurora/scheduler/async/TaskSchedulerTest.java a747f2b1ecbad7263931aeec3b12711096d2469d
>
> Diff: https://reviews.apache.org/r/16232/diff/
>
>
> Testing
> -------
>
> ./gradlew clean build
>
>
> Thanks,
>
> Zameer Manji
>
>