> On Feb. 14, 2014, 8:49 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/MesosSchedulerImpl.java, line 168
> > <https://reviews.apache.org/r/18141/diff/1/?file=486030#file486030line168>
> >
> >     This comment seems more appropriate for the SchedulerModule where the 
> > list is created.

I agree that it should be moved, but argue that the constructor doc is the most 
appropriate place.  Moved there, let me know if you feel otherwise.


> On Feb. 14, 2014, 8:49 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/periodic/GcExecutorLauncher.java, 
> > line 174
> > <https://reviews.apache.org/r/18141/diff/1/?file=486035#file486035line174>
> >
> >     Any particular reason you have two makeGcTask overloads? Is it just to 
> > make unit tests easier?

Yes.  This allows me to build expected TaskInfo messages without duplicating 
the ExecutorInfo and TaskInfo building code.


> On Feb. 14, 2014, 8:49 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/periodic/GcExecutorLauncher.java, 
> > line 199
> > <https://reviews.apache.org/r/18141/diff/1/?file=486035#file486035line199>
> >
> >     With the async execution there is a potential of leaking offers in case 
> > task creation starts failing for any reason. Would it make sense to have a 
> > separate stat counter (e.g. scheduler_gc_offers_used) to help 
> > troubleshooting such cases? Any difference between it and the 
> > scheduler_gc_tasks_created would be a red flag.

Done.


> On Feb. 14, 2014, 8:49 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/periodic/GcExecutorLauncher.java, 
> > line 270
> > <https://reviews.apache.org/r/18141/diff/1/?file=486035#file486035line270>
> >
> >     s/public//

Done, and i assume you'd like the same for the class.


> On Feb. 14, 2014, 8:49 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/periodic/GcExecutorLauncher.java, 
> > line 277
> > <https://reviews.apache.org/r/18141/diff/1/?file=486035#file486035line277>
> >
> >     @Override

Thanks, fixed.


- Bill


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18141/#review34542
-----------------------------------------------------------


On Feb. 14, 2014, 8:09 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18141/
> -----------------------------------------------------------
> 
> (Updated Feb. 14, 2014, 8:09 p.m.)
> 
> 
> Review request for Aurora, Deprecated Use kevints and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-214
>     https://issues.apache.org/jira/browse/AURORA-214
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This change also alters the signature to and renames TaskLauncher#createTask, 
> since no remaining TaskLaunchers return a task.  The signature change 
> resulted in removing MesosSchedulerImpl#fitsInOffer, which turned out to be 
> redundant in practice (GcExecutorLauncher did this internally).
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/MesosSchedulerImpl.java 
> 70ac62e6ba3566a8789f1c7b2042b8a94f5f3c02 
>   src/main/java/org/apache/aurora/scheduler/SchedulerModule.java 
> 92399cc7a38f0ddce8338d127fb7b579606f2571 
>   src/main/java/org/apache/aurora/scheduler/TaskLauncher.java 
> 96a3adee537aa87402a39dba090c6bbebe6afb1f 
>   src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 
> 010776efe1620c13bbcff611f9c1c3272d6c776f 
>   src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java 
> e7a5a8377f4c8a42012e7ecb118c597a2804da40 
>   src/main/java/org/apache/aurora/scheduler/periodic/GcExecutorLauncher.java 
> f0d4fbcc411dcb3642c21f51f65c89ad24c3400a 
>   src/test/java/org/apache/aurora/scheduler/MesosSchedulerImplTest.java 
> 92c77d53f09c39e710bd1bb3277f32d1e144e62f 
>   src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 
> ecf4f90c7d635f912f90fb367dbaa11401048052 
>   
> src/test/java/org/apache/aurora/scheduler/periodic/GcExecutorLauncherTest.java
>  98f5aa141ecca7475274d84f50c750db6f2908b5 
> 
> Diff: https://reviews.apache.org/r/18141/diff/
> 
> 
> Testing
> -------
> 
> $ ./gradlew build
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Reply via email to