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