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