You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Maxim Khutornenko <ma...@apache.org> on 2015/06/23 16:29:25 UTC
Review Request 35761: Refactoring TaskLauncher.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35761/
-----------------------------------------------------------
Review request for Aurora and Bill Farner.
Bugs: AURORA-1334
https://issues.apache.org/jira/browse/AURORA-1334
Repository: aurora
Description
-------
Replacing TaskLauncher abstraction with TaskStatusUpdateManager.
Will not apply cleanly. Depends on https://reviews.apache.org/r/35760/.
Diffs
-----
src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 4c63cc7af2fb5702fa649ce2cd4ee619139223e2
src/main/java/org/apache/aurora/scheduler/SchedulerModule.java 6edec22aba135136c4ce4066b9535f23de077db7
src/main/java/org/apache/aurora/scheduler/TaskLauncher.java cd55a6ee7424873c3e615d95422c9ecab6442f46
src/main/java/org/apache/aurora/scheduler/TaskStatusUpdateManager.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 6bfbf0c76399d569ac762a7b433a6c576bf0ee87
src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java 8bcac6c4f93eaca7250d43ab81a72102b91bf836
src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java 4f7a1bec8c32aef4b5f435c760f69de13888ea9d
src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 126001a90c819f9b3bfd448a060102bcef8b2a35
src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java e4e158721e0bdc7ed8a02eea24f4915ab937b9c7
Diff: https://reviews.apache.org/r/35761/diff/
Testing
-------
Thanks,
Maxim Khutornenko
Re: Review Request 35761: Refactoring TaskLauncher.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On June 23, 2015, 5:55 p.m., Bill Farner wrote:
> > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java, line 167
> > <https://reviews.apache.org/r/35761/diff/1/?file=990578#file990578line167>
> >
> > Accessing the Impl suggests an encapsulagion problem. Can you use the interface instead?
Yes, changed to AbstractExecutionThreadService.
> On June 23, 2015, 5:55 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/TaskStatusUpdateManager.java, line 21
> > <https://reviews.apache.org/r/35761/diff/1/?file=990581#file990581line21>
> >
> > How about `TaskStatusReceiver` or `TaskStatusHandler`?
Going with TaskStatusHandler as it was on my list as well.
> On June 23, 2015, 5:55 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java, line 176
> > <https://reviews.apache.org/r/35761/diff/1/?file=990583#file990583line176>
> >
> > Please include a comment explaining the rationale behind the default.
Done.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35761/#review89008
-----------------------------------------------------------
On June 23, 2015, 2:29 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35761/
> -----------------------------------------------------------
>
> (Updated June 23, 2015, 2:29 p.m.)
>
>
> Review request for Aurora and Bill Farner.
>
>
> Bugs: AURORA-1334
> https://issues.apache.org/jira/browse/AURORA-1334
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Replacing TaskLauncher abstraction with TaskStatusUpdateManager.
>
> Will not apply cleanly. Depends on https://reviews.apache.org/r/35760/.
>
>
> Diffs
> -----
>
> src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 4c63cc7af2fb5702fa649ce2cd4ee619139223e2
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java 6edec22aba135136c4ce4066b9535f23de077db7
> src/main/java/org/apache/aurora/scheduler/TaskLauncher.java cd55a6ee7424873c3e615d95422c9ecab6442f46
> src/main/java/org/apache/aurora/scheduler/TaskStatusUpdateManager.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 6bfbf0c76399d569ac762a7b433a6c576bf0ee87
> src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java 8bcac6c4f93eaca7250d43ab81a72102b91bf836
> src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java 4f7a1bec8c32aef4b5f435c760f69de13888ea9d
> src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 126001a90c819f9b3bfd448a060102bcef8b2a35
> src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java e4e158721e0bdc7ed8a02eea24f4915ab937b9c7
>
> Diff: https://reviews.apache.org/r/35761/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 35761: Refactoring TaskLauncher.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35761/#review89008
-----------------------------------------------------------
LGTM overall, will give a ship once the rebase comes out green.
src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java (line 164)
<https://reviews.apache.org/r/35761/#comment141605>
Accessing the Impl suggests an encapsulagion problem. Can you use the interface instead?
src/main/java/org/apache/aurora/scheduler/TaskStatusUpdateManager.java (line 21)
<https://reviews.apache.org/r/35761/#comment141604>
How about `TaskStatusReceiver` or `TaskStatusHandler`?
src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java (line 176)
<https://reviews.apache.org/r/35761/#comment141608>
Please include a comment explaining the rationale behind the default.
- Bill Farner
On June 23, 2015, 2:29 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35761/
> -----------------------------------------------------------
>
> (Updated June 23, 2015, 2:29 p.m.)
>
>
> Review request for Aurora and Bill Farner.
>
>
> Bugs: AURORA-1334
> https://issues.apache.org/jira/browse/AURORA-1334
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Replacing TaskLauncher abstraction with TaskStatusUpdateManager.
>
> Will not apply cleanly. Depends on https://reviews.apache.org/r/35760/.
>
>
> Diffs
> -----
>
> src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 4c63cc7af2fb5702fa649ce2cd4ee619139223e2
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java 6edec22aba135136c4ce4066b9535f23de077db7
> src/main/java/org/apache/aurora/scheduler/TaskLauncher.java cd55a6ee7424873c3e615d95422c9ecab6442f46
> src/main/java/org/apache/aurora/scheduler/TaskStatusUpdateManager.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 6bfbf0c76399d569ac762a7b433a6c576bf0ee87
> src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java 8bcac6c4f93eaca7250d43ab81a72102b91bf836
> src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java 4f7a1bec8c32aef4b5f435c760f69de13888ea9d
> src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 126001a90c819f9b3bfd448a060102bcef8b2a35
> src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java e4e158721e0bdc7ed8a02eea24f4915ab937b9c7
>
> Diff: https://reviews.apache.org/r/35761/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 35761: Refactoring TaskLauncher.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On June 23, 2015, 7:46 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java, line 242
> > <https://reviews.apache.org/r/35761/diff/2/?file=990689#file990689line242>
> >
> > s/manager/handler/
Done.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35761/#review89039
-----------------------------------------------------------
On June 23, 2015, 6:35 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35761/
> -----------------------------------------------------------
>
> (Updated June 23, 2015, 6:35 p.m.)
>
>
> Review request for Aurora and Bill Farner.
>
>
> Bugs: AURORA-1334
> https://issues.apache.org/jira/browse/AURORA-1334
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Replacing TaskLauncher abstraction with TaskStatusUpdateManager.
>
>
> Diffs
> -----
>
> src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 4c63cc7af2fb5702fa649ce2cd4ee619139223e2
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java ae31bdb587c0ea8380fc15c71fcf34e7e19405a6
> src/main/java/org/apache/aurora/scheduler/TaskLauncher.java cd55a6ee7424873c3e615d95422c9ecab6442f46
> src/main/java/org/apache/aurora/scheduler/TaskStatusHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 6bfbf0c76399d569ac762a7b433a6c576bf0ee87
> src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java 17b358533853596f0a577958a3a7303fa61bf299
> src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java 4f7a1bec8c32aef4b5f435c760f69de13888ea9d
> src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 126001a90c819f9b3bfd448a060102bcef8b2a35
> src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java e4e158721e0bdc7ed8a02eea24f4915ab937b9c7
>
> Diff: https://reviews.apache.org/r/35761/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 35761: Refactoring TaskLauncher.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35761/#review89039
-----------------------------------------------------------
Ship it!
src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java (line 233)
<https://reviews.apache.org/r/35761/#comment141643>
s/manager/handler/
- Bill Farner
On June 23, 2015, 6:35 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35761/
> -----------------------------------------------------------
>
> (Updated June 23, 2015, 6:35 p.m.)
>
>
> Review request for Aurora and Bill Farner.
>
>
> Bugs: AURORA-1334
> https://issues.apache.org/jira/browse/AURORA-1334
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Replacing TaskLauncher abstraction with TaskStatusUpdateManager.
>
>
> Diffs
> -----
>
> src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 4c63cc7af2fb5702fa649ce2cd4ee619139223e2
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java ae31bdb587c0ea8380fc15c71fcf34e7e19405a6
> src/main/java/org/apache/aurora/scheduler/TaskLauncher.java cd55a6ee7424873c3e615d95422c9ecab6442f46
> src/main/java/org/apache/aurora/scheduler/TaskStatusHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 6bfbf0c76399d569ac762a7b433a6c576bf0ee87
> src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java 17b358533853596f0a577958a3a7303fa61bf299
> src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java 4f7a1bec8c32aef4b5f435c760f69de13888ea9d
> src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 126001a90c819f9b3bfd448a060102bcef8b2a35
> src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java e4e158721e0bdc7ed8a02eea24f4915ab937b9c7
>
> Diff: https://reviews.apache.org/r/35761/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 35761: Refactoring TaskLauncher.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35761/
-----------------------------------------------------------
(Updated June 23, 2015, 8:39 p.m.)
Review request for Aurora and Bill Farner.
Changes
-------
Bill's comment.
Bugs: AURORA-1334
https://issues.apache.org/jira/browse/AURORA-1334
Repository: aurora
Description
-------
Replacing TaskLauncher abstraction with TaskStatusUpdateManager.
Diffs (updated)
-----
src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 4c63cc7af2fb5702fa649ce2cd4ee619139223e2
src/main/java/org/apache/aurora/scheduler/SchedulerModule.java ae31bdb587c0ea8380fc15c71fcf34e7e19405a6
src/main/java/org/apache/aurora/scheduler/TaskLauncher.java cd55a6ee7424873c3e615d95422c9ecab6442f46
src/main/java/org/apache/aurora/scheduler/TaskStatusHandler.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 6bfbf0c76399d569ac762a7b433a6c576bf0ee87
src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java 17b358533853596f0a577958a3a7303fa61bf299
src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java 4f7a1bec8c32aef4b5f435c760f69de13888ea9d
src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 126001a90c819f9b3bfd448a060102bcef8b2a35
src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java e4e158721e0bdc7ed8a02eea24f4915ab937b9c7
Diff: https://reviews.apache.org/r/35761/diff/
Testing
-------
Thanks,
Maxim Khutornenko
Re: Review Request 35761: Refactoring TaskLauncher.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35761/#review89026
-----------------------------------------------------------
Ship it!
Master (68c4620) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On June 23, 2015, 6:35 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35761/
> -----------------------------------------------------------
>
> (Updated June 23, 2015, 6:35 p.m.)
>
>
> Review request for Aurora and Bill Farner.
>
>
> Bugs: AURORA-1334
> https://issues.apache.org/jira/browse/AURORA-1334
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Replacing TaskLauncher abstraction with TaskStatusUpdateManager.
>
>
> Diffs
> -----
>
> src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 4c63cc7af2fb5702fa649ce2cd4ee619139223e2
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java ae31bdb587c0ea8380fc15c71fcf34e7e19405a6
> src/main/java/org/apache/aurora/scheduler/TaskLauncher.java cd55a6ee7424873c3e615d95422c9ecab6442f46
> src/main/java/org/apache/aurora/scheduler/TaskStatusHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 6bfbf0c76399d569ac762a7b433a6c576bf0ee87
> src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java 17b358533853596f0a577958a3a7303fa61bf299
> src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java 4f7a1bec8c32aef4b5f435c760f69de13888ea9d
> src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 126001a90c819f9b3bfd448a060102bcef8b2a35
> src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java e4e158721e0bdc7ed8a02eea24f4915ab937b9c7
>
> Diff: https://reviews.apache.org/r/35761/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 35761: Refactoring TaskLauncher.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35761/
-----------------------------------------------------------
(Updated June 23, 2015, 6:35 p.m.)
Review request for Aurora and Bill Farner.
Changes
-------
Rebased and addressed Bill's comments.
Bugs: AURORA-1334
https://issues.apache.org/jira/browse/AURORA-1334
Repository: aurora
Description (updated)
-------
Replacing TaskLauncher abstraction with TaskStatusUpdateManager.
Diffs (updated)
-----
src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 4c63cc7af2fb5702fa649ce2cd4ee619139223e2
src/main/java/org/apache/aurora/scheduler/SchedulerModule.java ae31bdb587c0ea8380fc15c71fcf34e7e19405a6
src/main/java/org/apache/aurora/scheduler/TaskLauncher.java cd55a6ee7424873c3e615d95422c9ecab6442f46
src/main/java/org/apache/aurora/scheduler/TaskStatusHandler.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 6bfbf0c76399d569ac762a7b433a6c576bf0ee87
src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java 17b358533853596f0a577958a3a7303fa61bf299
src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java 4f7a1bec8c32aef4b5f435c760f69de13888ea9d
src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 126001a90c819f9b3bfd448a060102bcef8b2a35
src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java e4e158721e0bdc7ed8a02eea24f4915ab937b9c7
Diff: https://reviews.apache.org/r/35761/diff/
Testing
-------
Thanks,
Maxim Khutornenko
Re: Review Request 35761: Refactoring TaskLauncher.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35761/#review88977
-----------------------------------------------------------
This patch does not apply cleanly on master (2df2db9), do you need to rebase?
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On June 23, 2015, 2:29 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35761/
> -----------------------------------------------------------
>
> (Updated June 23, 2015, 2:29 p.m.)
>
>
> Review request for Aurora and Bill Farner.
>
>
> Bugs: AURORA-1334
> https://issues.apache.org/jira/browse/AURORA-1334
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Replacing TaskLauncher abstraction with TaskStatusUpdateManager.
>
> Will not apply cleanly. Depends on https://reviews.apache.org/r/35760/.
>
>
> Diffs
> -----
>
> src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 4c63cc7af2fb5702fa649ce2cd4ee619139223e2
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java 6edec22aba135136c4ce4066b9535f23de077db7
> src/main/java/org/apache/aurora/scheduler/TaskLauncher.java cd55a6ee7424873c3e615d95422c9ecab6442f46
> src/main/java/org/apache/aurora/scheduler/TaskStatusUpdateManager.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 6bfbf0c76399d569ac762a7b433a6c576bf0ee87
> src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java 8bcac6c4f93eaca7250d43ab81a72102b91bf836
> src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java 4f7a1bec8c32aef4b5f435c760f69de13888ea9d
> src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 126001a90c819f9b3bfd448a060102bcef8b2a35
> src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java e4e158721e0bdc7ed8a02eea24f4915ab937b9c7
>
> Diff: https://reviews.apache.org/r/35761/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Maxim Khutornenko
>
>