Re: 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/ --- (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.
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.
--- 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.
--- 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
