> 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 > >
