You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Bill Farner <wf...@apache.org> on 2014/02/14 21:09:23 UTC
Review Request 18141: Prepare and launch GC executor tasks asynchronously.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18141/
-----------------------------------------------------------
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
Re: Review Request 18141: Prepare and launch GC executor tasks
asynchronously.
Posted by Bill Farner <wf...@apache.org>.
> 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
>
>
Re: Review Request 18141: Prepare and launch GC executor tasks
asynchronously.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18141/#review34542
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/MesosSchedulerImpl.java
<https://reviews.apache.org/r/18141/#comment64654>
This comment seems more appropriate for the SchedulerModule where the list is created.
src/main/java/org/apache/aurora/scheduler/periodic/GcExecutorLauncher.java
<https://reviews.apache.org/r/18141/#comment64655>
Any particular reason you have two makeGcTask overloads? Is it just to make unit tests easier?
src/main/java/org/apache/aurora/scheduler/periodic/GcExecutorLauncher.java
<https://reviews.apache.org/r/18141/#comment64656>
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.
src/main/java/org/apache/aurora/scheduler/periodic/GcExecutorLauncher.java
<https://reviews.apache.org/r/18141/#comment64657>
s/public//
src/main/java/org/apache/aurora/scheduler/periodic/GcExecutorLauncher.java
<https://reviews.apache.org/r/18141/#comment64658>
@Override
- Maxim Khutornenko
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
>
>
Re: Review Request 18141: Prepare and launch GC executor tasks
asynchronously.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18141/#review34749
-----------------------------------------------------------
Ship it!
Ship It!
- Maxim Khutornenko
On Feb. 18, 2014, 7:16 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18141/
> -----------------------------------------------------------
>
> (Updated Feb. 18, 2014, 7:16 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
>
>
Re: Review Request 18141: Prepare and launch GC executor tasks
asynchronously.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18141/#review35079
-----------------------------------------------------------
Ping? Kevin — waiting on a review from you.
- Bill Farner
On Feb. 19, 2014, 9:39 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18141/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2014, 9:39 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney 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
>
>
Re: Review Request 18141: Prepare and launch GC executor tasks
asynchronously.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18141/#review35103
-----------------------------------------------------------
Ship it!
Ship It!
- Kevin Sweeney
On Feb. 19, 2014, 1:39 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18141/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2014, 1:39 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney 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
>
>
Re: Review Request 18141: Prepare and launch GC executor tasks
asynchronously.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18141/
-----------------------------------------------------------
(Updated Feb. 19, 2014, 9:39 p.m.)
Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
Changes
-------
s/ksweeney/kevints/
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
Re: Review Request 18141: Prepare and launch GC executor tasks
asynchronously.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18141/
-----------------------------------------------------------
(Updated Feb. 18, 2014, 7:16 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 (updated)
-----
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