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 01:49:05 GMT


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


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