aurora-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zameer Manji" <zma...@gmail.com>
Subject Re: Review Request 16232: Add offer reservations to preemption flow
Date Sat, 14 Dec 2013 00:16:22 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message