aurora-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zameer Manji" <zma...@twopensource.com>
Subject Re: Review Request 16232: Add offer reservations to preemption flow
Date Tue, 17 Dec 2013 00:08:51 GMT


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


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