You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benno Evers <be...@yandex-team.ru> on 2017/09/11 09:16:52 UTC
Review Request 62212: Send TASK_STARTING from the built-in executors.
[1/2]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62212/
-----------------------------------------------------------
Review request for mesos, Andrei Budnik and Alexander Rukletsov.
Bugs: MESOS-7941
https://issues.apache.org/jira/browse/MESOS-7941
Repository: mesos
Description
-------
This gives schedulers more information about a tasks status,
in particular it gives a better estimate of a tasks start time
and helps differentiating between tasks stuck in TASK_STAGING
and tasks stuck in TASK_STARTING.
Diffs
-----
docs/high-availability-framework-guide.md 73743aba31f9d0ca827d318e2ecb4752a91b1be0
src/docker/executor.cpp e9949f652cd8527991ebfdfbf14e68b4c958fe79
src/launcher/default_executor.cpp 106b7f2e0244d211c66b237b5d1c51f43fc6e529
src/launcher/executor.cpp 951597b576b4912541dd87d52dcb981393e58082
Diff: https://reviews.apache.org/r/62212/diff/1/
Testing
-------
Thanks,
Benno Evers
Re: Review Request 62212: Send TASK_STARTING from the built-in
executors. [1/2]
Posted by Benno Evers <be...@yandex-team.ru>.
> On Sept. 12, 2017, 11:07 a.m., Alexander Rukletsov wrote:
> > src/launcher/default_executor.cpp
> > Lines 1317-1319 (original), 1333-1337 (patched)
> > <https://reviews.apache.org/r/62212/diff/1/?file=1819448#file1819448line1333>
> >
> > Instead of checking a flag, why not replacing `CHECK` with a condition?
I was assuming this `CHECK` was mostly intended to guard against some weird race conditions where this function might be called before `task_id` is inserted, even though we expect it to be. So it felt safer to keep the check. I don't have a strong opinion on this though, if you prefer I'll replace it.
- Benno
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62212/#review185157
-----------------------------------------------------------
On Sept. 11, 2017, 9:16 a.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62212/
> -----------------------------------------------------------
>
> (Updated Sept. 11, 2017, 9:16 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
>
>
> Bugs: MESOS-7941
> https://issues.apache.org/jira/browse/MESOS-7941
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This gives schedulers more information about a tasks status,
> in particular it gives a better estimate of a tasks start time
> and helps differentiating between tasks stuck in TASK_STAGING
> and tasks stuck in TASK_STARTING.
>
>
> Diffs
> -----
>
> docs/high-availability-framework-guide.md 73743aba31f9d0ca827d318e2ecb4752a91b1be0
> src/docker/executor.cpp e9949f652cd8527991ebfdfbf14e68b4c958fe79
> src/launcher/default_executor.cpp 106b7f2e0244d211c66b237b5d1c51f43fc6e529
> src/launcher/executor.cpp 951597b576b4912541dd87d52dcb981393e58082
>
>
> Diff: https://reviews.apache.org/r/62212/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 62212: Send TASK_STARTING from the built-in
executors. [1/2]
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62212/#review185157
-----------------------------------------------------------
For the default executor, a cleaner approach would be to initialize the `containers` collection earlier, right before (or together with) sending `TASK_STARTING`. If we rely on the fact that `containers` include only launched containers, we can introduce `active` flag into the `Container` struct and set it where now an instance of the struct is created. (Speaking about flags, we should have a proper state machine with clear transitions per container instead of a bunch of flags).
Moreover, from what I see in the code, if a launch nested container has failed, no task updates are sent by executor at all (which makes agent to create an implicit update). It seems reasonable to me to send an update from the executor at least for the task that has failed to launch. In the future we will likely allow some tasks in a group to fail without killing the complete group, hence keeping task info as early as possible seems reasonable to me.
src/docker/executor.cpp
Line 138 (original), 138-139 (patched)
<https://reviews.apache.org/r/62212/#comment261444>
Please add the todo here:
```
// TODO(alexr): Use `protobuf::createTaskStatus()`
// instead of manually setting fields.
```
src/launcher/default_executor.cpp
Lines 1256 (patched)
<https://reviews.apache.org/r/62212/#comment261446>
Instead of short-circuiting here, I suggest to use `protobuf::createTaskStatus` directly, explaining in the comment at the call site why we do so. There is another alternative, more about it in the review header section.
src/launcher/default_executor.cpp
Lines 1317-1319 (original), 1333-1337 (patched)
<https://reviews.apache.org/r/62212/#comment261447>
Instead of checking a flag, why not replacing `CHECK` with a condition?
- Alexander Rukletsov
On Sept. 11, 2017, 9:16 a.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62212/
> -----------------------------------------------------------
>
> (Updated Sept. 11, 2017, 9:16 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
>
>
> Bugs: MESOS-7941
> https://issues.apache.org/jira/browse/MESOS-7941
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This gives schedulers more information about a tasks status,
> in particular it gives a better estimate of a tasks start time
> and helps differentiating between tasks stuck in TASK_STAGING
> and tasks stuck in TASK_STARTING.
>
>
> Diffs
> -----
>
> docs/high-availability-framework-guide.md 73743aba31f9d0ca827d318e2ecb4752a91b1be0
> src/docker/executor.cpp e9949f652cd8527991ebfdfbf14e68b4c958fe79
> src/launcher/default_executor.cpp 106b7f2e0244d211c66b237b5d1c51f43fc6e529
> src/launcher/executor.cpp 951597b576b4912541dd87d52dcb981393e58082
>
>
> Diff: https://reviews.apache.org/r/62212/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 62212: Send TASK_STARTING from the built-in
executors. [1/2]
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Sept. 12, 2017, 11:03 a.m., Andrei Budnik wrote:
> > src/docker/executor.cpp
> > Lines 141 (patched)
> > <https://reviews.apache.org/r/62212/diff/1/?file=1819447#file1819447line141>
> >
> > Do we really need to send `TASK_STARTING`, if we know that right after sending `TASK_STARTING`, `launchTask` might send `TASK_FAILED`, e.g. in case of following checks:
> > ```c++
> > if (run.isSome()) {
> > // ...
> > status.set_state(TASK_FAILED);
> > }
> > ...
> > if (runOptions.isError()) {
> > // ...
> > status.set_state(TASK_FAILED);
> > }
> > ```
> > What is the semantics of `TASK_STARTING` - should an executor send it always and unconditionally, or can we omit `TASK_STARTING` when we are pretty sure that we won't be able to launch a task?
> > Omitting `TASK_STARTING` before sending `TASK_FAILED` might be a good optimization, if it's not important to send `TASK_STARTING` first.
Agree with Andrei: we should send task starting if we try to start the task. If we know that starting will never succeed, we don't even try, hence no task starting update : )
> On Sept. 12, 2017, 11:03 a.m., Andrei Budnik wrote:
> > src/launcher/executor.cpp
> > Lines 512 (patched)
> > <https://reviews.apache.org/r/62212/diff/1/?file=1819449#file1819449line512>
> >
> > What happens if a buggy scheduler will try to launch multiple tasks with same `task_id` using a single `command` executor? I guess, `command` executor will send `TASK_STARTING`, then `TASK_FAILED`. If so, then is it ok to have multiple `TASK_STARTING` status updates (having different timestamps)?
Multiple tasks for a single command executor are not allowed. We should send task starting only once, for the task that we actually try to start.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62212/#review185156
-----------------------------------------------------------
On Sept. 11, 2017, 9:16 a.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62212/
> -----------------------------------------------------------
>
> (Updated Sept. 11, 2017, 9:16 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
>
>
> Bugs: MESOS-7941
> https://issues.apache.org/jira/browse/MESOS-7941
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This gives schedulers more information about a tasks status,
> in particular it gives a better estimate of a tasks start time
> and helps differentiating between tasks stuck in TASK_STAGING
> and tasks stuck in TASK_STARTING.
>
>
> Diffs
> -----
>
> docs/high-availability-framework-guide.md 73743aba31f9d0ca827d318e2ecb4752a91b1be0
> src/docker/executor.cpp e9949f652cd8527991ebfdfbf14e68b4c958fe79
> src/launcher/default_executor.cpp 106b7f2e0244d211c66b237b5d1c51f43fc6e529
> src/launcher/executor.cpp 951597b576b4912541dd87d52dcb981393e58082
>
>
> Diff: https://reviews.apache.org/r/62212/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 62212: Send TASK_STARTING from the built-in
executors. [1/2]
Posted by Benno Evers <be...@yandex-team.ru>.
> On Sept. 12, 2017, 11:03 a.m., Andrei Budnik wrote:
> > src/docker/executor.cpp
> > Lines 141 (patched)
> > <https://reviews.apache.org/r/62212/diff/1/?file=1819447#file1819447line141>
> >
> > Do we really need to send `TASK_STARTING`, if we know that right after sending `TASK_STARTING`, `launchTask` might send `TASK_FAILED`, e.g. in case of following checks:
> > ```c++
> > if (run.isSome()) {
> > // ...
> > status.set_state(TASK_FAILED);
> > }
> > ...
> > if (runOptions.isError()) {
> > // ...
> > status.set_state(TASK_FAILED);
> > }
> > ```
> > What is the semantics of `TASK_STARTING` - should an executor send it always and unconditionally, or can we omit `TASK_STARTING` when we are pretty sure that we won't be able to launch a task?
> > Omitting `TASK_STARTING` before sending `TASK_FAILED` might be a good optimization, if it's not important to send `TASK_STARTING` first.
>
> Alexander Rukletsov wrote:
> Agree with Andrei: we should send task starting if we try to start the task. If we know that starting will never succeed, we don't even try, hence no task starting update : )
The previous documentation on `TASK_STARTING` was more of a loose guideline, so I think we're de-facto deciding on the `TASK_STARTING` semantics in this review. Intuitively, I find it most natural to have it mean "the executor is now aware of the task and will attempt to start it", and not to create any special cases.
- Benno
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62212/#review185156
-----------------------------------------------------------
On Sept. 11, 2017, 9:16 a.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62212/
> -----------------------------------------------------------
>
> (Updated Sept. 11, 2017, 9:16 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
>
>
> Bugs: MESOS-7941
> https://issues.apache.org/jira/browse/MESOS-7941
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This gives schedulers more information about a tasks status,
> in particular it gives a better estimate of a tasks start time
> and helps differentiating between tasks stuck in TASK_STAGING
> and tasks stuck in TASK_STARTING.
>
>
> Diffs
> -----
>
> docs/high-availability-framework-guide.md 73743aba31f9d0ca827d318e2ecb4752a91b1be0
> src/docker/executor.cpp e9949f652cd8527991ebfdfbf14e68b4c958fe79
> src/launcher/default_executor.cpp 106b7f2e0244d211c66b237b5d1c51f43fc6e529
> src/launcher/executor.cpp 951597b576b4912541dd87d52dcb981393e58082
>
>
> Diff: https://reviews.apache.org/r/62212/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 62212: Send TASK_STARTING from the built-in
executors. [1/2]
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On Sept. 12, 2017, 11:03 a.m., Andrei Budnik wrote:
> > src/docker/executor.cpp
> > Lines 141 (patched)
> > <https://reviews.apache.org/r/62212/diff/1/?file=1819447#file1819447line141>
> >
> > Do we really need to send `TASK_STARTING`, if we know that right after sending `TASK_STARTING`, `launchTask` might send `TASK_FAILED`, e.g. in case of following checks:
> > ```c++
> > if (run.isSome()) {
> > // ...
> > status.set_state(TASK_FAILED);
> > }
> > ...
> > if (runOptions.isError()) {
> > // ...
> > status.set_state(TASK_FAILED);
> > }
> > ```
> > What is the semantics of `TASK_STARTING` - should an executor send it always and unconditionally, or can we omit `TASK_STARTING` when we are pretty sure that we won't be able to launch a task?
> > Omitting `TASK_STARTING` before sending `TASK_FAILED` might be a good optimization, if it's not important to send `TASK_STARTING` first.
>
> Alexander Rukletsov wrote:
> Agree with Andrei: we should send task starting if we try to start the task. If we know that starting will never succeed, we don't even try, hence no task starting update : )
>
> Benno Evers wrote:
> The previous documentation on `TASK_STARTING` was more of a loose guideline, so I think we're de-facto deciding on the `TASK_STARTING` semantics in this review. Intuitively, I find it most natural to have it mean "the executor is now aware of the task and will attempt to start it", and not to create any special cases.
We have agreed that `TASK_STARTING` must be sent whenever an executor receives `launchTask`. So, I'm dropping this `Fix it!`.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62212/#review185156
-----------------------------------------------------------
On Sept. 11, 2017, 9:16 a.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62212/
> -----------------------------------------------------------
>
> (Updated Sept. 11, 2017, 9:16 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
>
>
> Bugs: MESOS-7941
> https://issues.apache.org/jira/browse/MESOS-7941
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This gives schedulers more information about a tasks status,
> in particular it gives a better estimate of a tasks start time
> and helps differentiating between tasks stuck in TASK_STAGING
> and tasks stuck in TASK_STARTING.
>
>
> Diffs
> -----
>
> docs/high-availability-framework-guide.md 73743aba31f9d0ca827d318e2ecb4752a91b1be0
> src/docker/executor.cpp e9949f652cd8527991ebfdfbf14e68b4c958fe79
> src/launcher/default_executor.cpp 106b7f2e0244d211c66b237b5d1c51f43fc6e529
> src/launcher/executor.cpp 951597b576b4912541dd87d52dcb981393e58082
>
>
> Diff: https://reviews.apache.org/r/62212/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 62212: Send TASK_STARTING from the built-in
executors. [1/2]
Posted by Andrei Budnik <ab...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62212/#review185156
-----------------------------------------------------------
src/docker/executor.cpp
Lines 141 (patched)
<https://reviews.apache.org/r/62212/#comment261443>
Do we really need to send `TASK_STARTING`, if we know that right after sending `TASK_STARTING`, `launchTask` might send `TASK_FAILED`, e.g. in case of following checks:
```c++
if (run.isSome()) {
// ...
status.set_state(TASK_FAILED);
}
...
if (runOptions.isError()) {
// ...
status.set_state(TASK_FAILED);
}
```
What is the semantics of `TASK_STARTING` - should an executor send it always and unconditionally, or can we omit `TASK_STARTING` when we are pretty sure that we won't be able to launch a task?
Omitting `TASK_STARTING` before sending `TASK_FAILED` might be a good optimization, if it's not important to send `TASK_STARTING` first.
src/launcher/default_executor.cpp
Lines 379 (patched)
<https://reviews.apache.org/r/62212/#comment261448>
Why `DefaultExecutor::createTaskStatus` used instead of `protobuf::createTaskStatus`?
Do we really need to set `executor_id` for `TASK_STARTING` status update? If so, then why don't we set `executor_id` in other built-in executors for consistency?
src/launcher/executor.cpp
Lines 512 (patched)
<https://reviews.apache.org/r/62212/#comment261445>
What happens if a buggy scheduler will try to launch multiple tasks with same `task_id` using a single `command` executor? I guess, `command` executor will send `TASK_STARTING`, then `TASK_FAILED`. If so, then is it ok to have multiple `TASK_STARTING` status updates (having different timestamps)?
- Andrei Budnik
On Sept. 11, 2017, 9:16 a.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62212/
> -----------------------------------------------------------
>
> (Updated Sept. 11, 2017, 9:16 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
>
>
> Bugs: MESOS-7941
> https://issues.apache.org/jira/browse/MESOS-7941
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This gives schedulers more information about a tasks status,
> in particular it gives a better estimate of a tasks start time
> and helps differentiating between tasks stuck in TASK_STAGING
> and tasks stuck in TASK_STARTING.
>
>
> Diffs
> -----
>
> docs/high-availability-framework-guide.md 73743aba31f9d0ca827d318e2ecb4752a91b1be0
> src/docker/executor.cpp e9949f652cd8527991ebfdfbf14e68b4c958fe79
> src/launcher/default_executor.cpp 106b7f2e0244d211c66b237b5d1c51f43fc6e529
> src/launcher/executor.cpp 951597b576b4912541dd87d52dcb981393e58082
>
>
> Diff: https://reviews.apache.org/r/62212/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 62212: Send TASK_STARTING from the built-in
executors.
Posted by Benno Evers <be...@yandex-team.ru>.
> On Sept. 13, 2017, 5:29 p.m., Andrei Budnik wrote:
> > src/launcher/default_executor.cpp
> > Line 326 (original), 332 (patched)
> > <https://reviews.apache.org/r/62212/diff/2/?file=1820838#file1820838line332>
> >
> > Should we update the comment above?
I think the comment is still correct.
> On Sept. 13, 2017, 5:29 p.m., Andrei Budnik wrote:
> > src/launcher/default_executor.cpp
> > Lines 412 (patched)
> > <https://reviews.apache.org/r/62212/diff/2/?file=1820838#file1820838line412>
> >
> > Adding default constructor for `DefaultExecutor::Container` will help us to get rid of repeating `None()` and `false`.
I'm not sure about adding a function that will be called only once.
- Benno
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62212/#review185310
-----------------------------------------------------------
On Sept. 13, 2017, 2:52 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62212/
> -----------------------------------------------------------
>
> (Updated Sept. 13, 2017, 2:52 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
>
>
> Bugs: MESOS-7941
> https://issues.apache.org/jira/browse/MESOS-7941
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This gives schedulers more information about a tasks status,
> in particular it gives a better estimate of a tasks start time
> and helps differentiating between tasks stuck in TASK_STAGING
> and tasks stuck in TASK_STARTING.
>
>
> Diffs
> -----
>
> docs/high-availability-framework-guide.md 73743aba31f9d0ca827d318e2ecb4752a91b1be0
> src/docker/executor.cpp e9949f652cd8527991ebfdfbf14e68b4c958fe79
> src/launcher/default_executor.cpp 106b7f2e0244d211c66b237b5d1c51f43fc6e529
> src/launcher/executor.cpp 951597b576b4912541dd87d52dcb981393e58082
>
>
> Diff: https://reviews.apache.org/r/62212/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 62212: Send TASK_STARTING from the built-in
executors.
Posted by Andrei Budnik <ab...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62212/#review185310
-----------------------------------------------------------
src/docker/executor.cpp
Lines 160 (patched)
<https://reviews.apache.org/r/62212/#comment261616>
Consider moving this comment right after `LOG(INFO) << "Starting task " << taskId.get();`.
src/launcher/default_executor.cpp
Line 326 (original), 332 (patched)
<https://reviews.apache.org/r/62212/#comment261617>
Should we update the comment above?
src/launcher/default_executor.cpp
Lines 412 (patched)
<https://reviews.apache.org/r/62212/#comment261618>
Adding default constructor for `DefaultExecutor::Container` will help us to get rid of repeating `None()` and `false`.
src/launcher/executor.cpp
Line 1019 (original), 1023 (patched)
<https://reviews.apache.org/r/62212/#comment261612>
As you have moved
```c++
TaskStatus starting = createTaskStatus(taskId.get(), TASK_STARTING);
forward(starting);
```
after `taskData = TaskData(task);`, this change is not necessarry anymore and can be reverted.
- Andrei Budnik
On Sept. 13, 2017, 2:52 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62212/
> -----------------------------------------------------------
>
> (Updated Sept. 13, 2017, 2:52 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
>
>
> Bugs: MESOS-7941
> https://issues.apache.org/jira/browse/MESOS-7941
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This gives schedulers more information about a tasks status,
> in particular it gives a better estimate of a tasks start time
> and helps differentiating between tasks stuck in TASK_STAGING
> and tasks stuck in TASK_STARTING.
>
>
> Diffs
> -----
>
> docs/high-availability-framework-guide.md 73743aba31f9d0ca827d318e2ecb4752a91b1be0
> src/docker/executor.cpp e9949f652cd8527991ebfdfbf14e68b4c958fe79
> src/launcher/default_executor.cpp 106b7f2e0244d211c66b237b5d1c51f43fc6e529
> src/launcher/executor.cpp 951597b576b4912541dd87d52dcb981393e58082
>
>
> Diff: https://reviews.apache.org/r/62212/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 62212: Send TASK_STARTING from the built-in
executors.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62212/#review187808
-----------------------------------------------------------
Ship it!
Ship It!
- Alexander Rukletsov
On Oct. 12, 2017, 11:20 a.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62212/
> -----------------------------------------------------------
>
> (Updated Oct. 12, 2017, 11:20 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
>
>
> Bugs: MESOS-7941
> https://issues.apache.org/jira/browse/MESOS-7941
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This gives schedulers more information about a tasks status,
> in particular it gives a better estimate of a tasks start time
> and helps differentiating between tasks stuck in TASK_STAGING
> and tasks stuck in TASK_STARTING.
>
>
> Diffs
> -----
>
> docs/high-availability-framework-guide.md 73743aba31f9d0ca827d318e2ecb4752a91b1be0
> src/docker/executor.cpp 3b0767ffbbe9982639d0b87fbb0573accdd48559
> src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0
> src/launcher/executor.cpp 0131577d5b9018e7094815f1377b2ee59a4493d4
>
>
> Diff: https://reviews.apache.org/r/62212/diff/6/
>
>
> Testing
> -------
>
> (for the complete chain starting with this RR)
> - `make check`
> - Installed in local DC/OS cluster and successfully completed the tweeter tutorial
> - Installed in local DC/OS cluster and successfully ran a periodic sleep task using chronos.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 62212: Send TASK_STARTING from the built-in
executors.
Posted by Andrei Budnik <ab...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62212/#review187772
-----------------------------------------------------------
Ship it!
Ship It!
- Andrei Budnik
On Oct. 12, 2017, 11:20 a.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62212/
> -----------------------------------------------------------
>
> (Updated Oct. 12, 2017, 11:20 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
>
>
> Bugs: MESOS-7941
> https://issues.apache.org/jira/browse/MESOS-7941
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This gives schedulers more information about a tasks status,
> in particular it gives a better estimate of a tasks start time
> and helps differentiating between tasks stuck in TASK_STAGING
> and tasks stuck in TASK_STARTING.
>
>
> Diffs
> -----
>
> docs/high-availability-framework-guide.md 73743aba31f9d0ca827d318e2ecb4752a91b1be0
> src/docker/executor.cpp 3b0767ffbbe9982639d0b87fbb0573accdd48559
> src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0
> src/launcher/executor.cpp 0131577d5b9018e7094815f1377b2ee59a4493d4
>
>
> Diff: https://reviews.apache.org/r/62212/diff/6/
>
>
> Testing
> -------
>
> (for the complete chain starting with this RR)
> - `make check`
> - Installed in local DC/OS cluster and successfully completed the tweeter tutorial
> - Installed in local DC/OS cluster and successfully ran a periodic sleep task using chronos.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 62212: Send TASK_STARTING from the built-in
executors.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62212/#review188554
-----------------------------------------------------------
Ship it!
Ship It!
- Alexander Rukletsov
On Oct. 12, 2017, 11:20 a.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62212/
> -----------------------------------------------------------
>
> (Updated Oct. 12, 2017, 11:20 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
>
>
> Bugs: MESOS-7941
> https://issues.apache.org/jira/browse/MESOS-7941
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This gives schedulers more information about a tasks status,
> in particular it gives a better estimate of a tasks start time
> and helps differentiating between tasks stuck in TASK_STAGING
> and tasks stuck in TASK_STARTING.
>
>
> Diffs
> -----
>
> docs/high-availability-framework-guide.md 73743aba31f9d0ca827d318e2ecb4752a91b1be0
> src/docker/executor.cpp 3b0767ffbbe9982639d0b87fbb0573accdd48559
> src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0
> src/launcher/executor.cpp 0131577d5b9018e7094815f1377b2ee59a4493d4
>
>
> Diff: https://reviews.apache.org/r/62212/diff/6/
>
>
> Testing
> -------
>
> (for the complete chain starting with this RR)
> - `make check`
> - Installed in local DC/OS cluster and successfully completed the tweeter tutorial
> - Installed in local DC/OS cluster and successfully ran a periodic sleep task using chronos.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 62212: Send TASK_STARTING from the built-in
executors.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62212/
-----------------------------------------------------------
(Updated Oct. 12, 2017, 11:20 a.m.)
Review request for mesos, Andrei Budnik and Alexander Rukletsov.
Bugs: MESOS-7941
https://issues.apache.org/jira/browse/MESOS-7941
Repository: mesos
Description
-------
This gives schedulers more information about a tasks status,
in particular it gives a better estimate of a tasks start time
and helps differentiating between tasks stuck in TASK_STAGING
and tasks stuck in TASK_STARTING.
Diffs (updated)
-----
docs/high-availability-framework-guide.md 73743aba31f9d0ca827d318e2ecb4752a91b1be0
src/docker/executor.cpp 3b0767ffbbe9982639d0b87fbb0573accdd48559
src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0
src/launcher/executor.cpp 0131577d5b9018e7094815f1377b2ee59a4493d4
Diff: https://reviews.apache.org/r/62212/diff/6/
Changes: https://reviews.apache.org/r/62212/diff/5-6/
Testing
-------
(for the complete chain starting with this RR)
- `make check`
- Installed in local DC/OS cluster and successfully completed the tweeter tutorial
- Installed in local DC/OS cluster and successfully ran a periodic sleep task using chronos.
Thanks,
Benno Evers
Re: Review Request 62212: Send TASK_STARTING from the built-in
executors.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62212/
-----------------------------------------------------------
(Updated Oct. 12, 2017, 12:11 a.m.)
Review request for mesos, Andrei Budnik and Alexander Rukletsov.
Bugs: MESOS-7941
https://issues.apache.org/jira/browse/MESOS-7941
Repository: mesos
Description
-------
This gives schedulers more information about a tasks status,
in particular it gives a better estimate of a tasks start time
and helps differentiating between tasks stuck in TASK_STAGING
and tasks stuck in TASK_STARTING.
Diffs (updated)
-----
docs/high-availability-framework-guide.md 73743aba31f9d0ca827d318e2ecb4752a91b1be0
src/docker/executor.cpp 3b0767ffbbe9982639d0b87fbb0573accdd48559
src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0
src/launcher/executor.cpp 0131577d5b9018e7094815f1377b2ee59a4493d4
Diff: https://reviews.apache.org/r/62212/diff/5/
Changes: https://reviews.apache.org/r/62212/diff/4-5/
Testing
-------
(for the complete chain starting with this RR)
- `make check`
- Installed in local DC/OS cluster and successfully completed the tweeter tutorial
- Installed in local DC/OS cluster and successfully ran a periodic sleep task using chronos.
Thanks,
Benno Evers
Re: Review Request 62212: Send TASK_STARTING from the built-in
executors.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62212/
-----------------------------------------------------------
(Updated Oct. 11, 2017, 11:18 p.m.)
Review request for mesos, Andrei Budnik and Alexander Rukletsov.
Bugs: MESOS-7941
https://issues.apache.org/jira/browse/MESOS-7941
Repository: mesos
Description
-------
This gives schedulers more information about a tasks status,
in particular it gives a better estimate of a tasks start time
and helps differentiating between tasks stuck in TASK_STAGING
and tasks stuck in TASK_STARTING.
Diffs
-----
docs/high-availability-framework-guide.md 73743aba31f9d0ca827d318e2ecb4752a91b1be0
src/docker/executor.cpp 3b0767ffbbe9982639d0b87fbb0573accdd48559
src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0
src/launcher/executor.cpp 0131577d5b9018e7094815f1377b2ee59a4493d4
Diff: https://reviews.apache.org/r/62212/diff/4/
Testing (updated)
-------
(for the complete chain starting with this RR)
- `make check`
- Installed in local DC/OS cluster and successfully completed the tweeter tutorial
- Installed in local DC/OS cluster and successfully ran a periodic sleep task using chronos.
Thanks,
Benno Evers
Re: Review Request 62212: Send TASK_STARTING from the built-in
executors.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62212/
-----------------------------------------------------------
(Updated Oct. 11, 2017, 11:18 p.m.)
Review request for mesos, Andrei Budnik and Alexander Rukletsov.
Bugs: MESOS-7941
https://issues.apache.org/jira/browse/MESOS-7941
Repository: mesos
Description
-------
This gives schedulers more information about a tasks status,
in particular it gives a better estimate of a tasks start time
and helps differentiating between tasks stuck in TASK_STAGING
and tasks stuck in TASK_STARTING.
Diffs
-----
docs/high-availability-framework-guide.md 73743aba31f9d0ca827d318e2ecb4752a91b1be0
src/docker/executor.cpp 3b0767ffbbe9982639d0b87fbb0573accdd48559
src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0
src/launcher/executor.cpp 0131577d5b9018e7094815f1377b2ee59a4493d4
Diff: https://reviews.apache.org/r/62212/diff/4/
Testing (updated)
-------
(for the complete chain starting with this RR)
* `make check`
* Installed in local DC/OS cluster and successfully completed the tweeter tutorial
* Installed in local DC/OS cluster and successfully ran a periodic sleep task using chronos.
Thanks,
Benno Evers
Re: Review Request 62212: Send TASK_STARTING from the built-in
executors.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62212/#review187711
-----------------------------------------------------------
src/launcher/default_executor.cpp
Lines 489-490 (patched)
<https://reviews.apache.org/r/62212/#comment264760>
You don't need to explicitly call out `None()`. As a bonus, you can fit it in one line : )
src/launcher/default_executor.cpp
Lines 528-529 (original), 555-556 (patched)
<https://reviews.apache.org/r/62212/#comment264758>
Please check the internal invariant:
`CHECK(containers.contains(taskId));`
- Alexander Rukletsov
On Oct. 11, 2017, 10:28 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62212/
> -----------------------------------------------------------
>
> (Updated Oct. 11, 2017, 10:28 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
>
>
> Bugs: MESOS-7941
> https://issues.apache.org/jira/browse/MESOS-7941
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This gives schedulers more information about a tasks status,
> in particular it gives a better estimate of a tasks start time
> and helps differentiating between tasks stuck in TASK_STAGING
> and tasks stuck in TASK_STARTING.
>
>
> Diffs
> -----
>
> docs/high-availability-framework-guide.md 73743aba31f9d0ca827d318e2ecb4752a91b1be0
> src/docker/executor.cpp 3b0767ffbbe9982639d0b87fbb0573accdd48559
> src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0
> src/launcher/executor.cpp 0131577d5b9018e7094815f1377b2ee59a4493d4
>
>
> Diff: https://reviews.apache.org/r/62212/diff/4/
>
>
> Testing
> -------
>
> * `make check`
> * Installed in local DC/OS cluster and successfully completed the tweeter tutorial
> * Installed in local DC/OS cluster and successfully ran a periodic sleep task using chronos.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 62212: Send TASK_STARTING from the built-in
executors.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62212/
-----------------------------------------------------------
(Updated Oct. 11, 2017, 10:28 p.m.)
Review request for mesos, Andrei Budnik and Alexander Rukletsov.
Bugs: MESOS-7941
https://issues.apache.org/jira/browse/MESOS-7941
Repository: mesos
Description
-------
This gives schedulers more information about a tasks status,
in particular it gives a better estimate of a tasks start time
and helps differentiating between tasks stuck in TASK_STAGING
and tasks stuck in TASK_STARTING.
Diffs
-----
docs/high-availability-framework-guide.md 73743aba31f9d0ca827d318e2ecb4752a91b1be0
src/docker/executor.cpp 3b0767ffbbe9982639d0b87fbb0573accdd48559
src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0
src/launcher/executor.cpp 0131577d5b9018e7094815f1377b2ee59a4493d4
Diff: https://reviews.apache.org/r/62212/diff/4/
Testing (updated)
-------
* `make check`
* Installed in local DC/OS cluster and successfully completed the tweeter tutorial
* Installed in local DC/OS cluster and successfully ran a periodic sleep task using chronos.
Thanks,
Benno Evers
Re: Review Request 62212: Send TASK_STARTING from the built-in
executors.
Posted by Benno Evers <be...@yandex-team.ru>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62212/
-----------------------------------------------------------
(Updated Sept. 29, 2017, 7:48 p.m.)
Review request for mesos, Andrei Budnik and Alexander Rukletsov.
Bugs: MESOS-7941
https://issues.apache.org/jira/browse/MESOS-7941
Repository: mesos
Description
-------
This gives schedulers more information about a tasks status,
in particular it gives a better estimate of a tasks start time
and helps differentiating between tasks stuck in TASK_STAGING
and tasks stuck in TASK_STARTING.
Diffs (updated)
-----
docs/high-availability-framework-guide.md 73743aba31f9d0ca827d318e2ecb4752a91b1be0
src/docker/executor.cpp 3b0767ffbbe9982639d0b87fbb0573accdd48559
src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0
src/launcher/executor.cpp 0131577d5b9018e7094815f1377b2ee59a4493d4
Diff: https://reviews.apache.org/r/62212/diff/3/
Changes: https://reviews.apache.org/r/62212/diff/2-3/
Testing
-------
Thanks,
Benno Evers
Re: Review Request 62212: Send TASK_STARTING from the built-in
executors.
Posted by Benno Evers <be...@yandex-team.ru>.
> On Sept. 15, 2017, 11:36 a.m., Andrei Budnik wrote:
> > src/launcher/default_executor.cpp
> > Line 1379 (original), 1396 (patched)
> > <https://reviews.apache.org/r/62212/diff/2/?file=1820838#file1820838line1406>
> >
> > If a default executor is allowed to call `launchGroup` more than once, then there may be a case when some containers are launched already, while others not.
I think if this is possible, the error was already in the existing code?
- Benno
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62212/#review185478
-----------------------------------------------------------
On Sept. 13, 2017, 2:52 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62212/
> -----------------------------------------------------------
>
> (Updated Sept. 13, 2017, 2:52 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
>
>
> Bugs: MESOS-7941
> https://issues.apache.org/jira/browse/MESOS-7941
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This gives schedulers more information about a tasks status,
> in particular it gives a better estimate of a tasks start time
> and helps differentiating between tasks stuck in TASK_STAGING
> and tasks stuck in TASK_STARTING.
>
>
> Diffs
> -----
>
> docs/high-availability-framework-guide.md 73743aba31f9d0ca827d318e2ecb4752a91b1be0
> src/docker/executor.cpp e9949f652cd8527991ebfdfbf14e68b4c958fe79
> src/launcher/default_executor.cpp 106b7f2e0244d211c66b237b5d1c51f43fc6e529
> src/launcher/executor.cpp 951597b576b4912541dd87d52dcb981393e58082
>
>
> Diff: https://reviews.apache.org/r/62212/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 62212: Send TASK_STARTING from the built-in
executors.
Posted by Andrei Budnik <ab...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62212/#review185478
-----------------------------------------------------------
src/launcher/default_executor.cpp
Line 1379 (original), 1396 (patched)
<https://reviews.apache.org/r/62212/#comment261769>
If a default executor is allowed to call `launchGroup` more than once, then there may be a case when some containers are launched already, while others not.
- Andrei Budnik
On Sept. 13, 2017, 2:52 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62212/
> -----------------------------------------------------------
>
> (Updated Sept. 13, 2017, 2:52 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
>
>
> Bugs: MESOS-7941
> https://issues.apache.org/jira/browse/MESOS-7941
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This gives schedulers more information about a tasks status,
> in particular it gives a better estimate of a tasks start time
> and helps differentiating between tasks stuck in TASK_STAGING
> and tasks stuck in TASK_STARTING.
>
>
> Diffs
> -----
>
> docs/high-availability-framework-guide.md 73743aba31f9d0ca827d318e2ecb4752a91b1be0
> src/docker/executor.cpp e9949f652cd8527991ebfdfbf14e68b4c958fe79
> src/launcher/default_executor.cpp 106b7f2e0244d211c66b237b5d1c51f43fc6e529
> src/launcher/executor.cpp 951597b576b4912541dd87d52dcb981393e58082
>
>
> Diff: https://reviews.apache.org/r/62212/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 62212: Send TASK_STARTING from the built-in
executors.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62212/#review185566
-----------------------------------------------------------
src/launcher/default_executor.cpp
Lines 486-492 (patched)
<https://reviews.apache.org/r/62212/#comment261883>
Why don't you send the update right after creating a `Container` instance?
- Alexander Rukletsov
On Sept. 13, 2017, 2:52 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62212/
> -----------------------------------------------------------
>
> (Updated Sept. 13, 2017, 2:52 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
>
>
> Bugs: MESOS-7941
> https://issues.apache.org/jira/browse/MESOS-7941
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This gives schedulers more information about a tasks status,
> in particular it gives a better estimate of a tasks start time
> and helps differentiating between tasks stuck in TASK_STAGING
> and tasks stuck in TASK_STARTING.
>
>
> Diffs
> -----
>
> docs/high-availability-framework-guide.md 73743aba31f9d0ca827d318e2ecb4752a91b1be0
> src/docker/executor.cpp e9949f652cd8527991ebfdfbf14e68b4c958fe79
> src/launcher/default_executor.cpp 106b7f2e0244d211c66b237b5d1c51f43fc6e529
> src/launcher/executor.cpp 951597b576b4912541dd87d52dcb981393e58082
>
>
> Diff: https://reviews.apache.org/r/62212/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 62212: Send TASK_STARTING from the built-in
executors.
Posted by Benno Evers <be...@yandex-team.ru>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62212/
-----------------------------------------------------------
(Updated Sept. 13, 2017, 2:52 p.m.)
Review request for mesos, Andrei Budnik and Alexander Rukletsov.
Changes
-------
Send task starting slightly later; initialize containers slightly earlier
Summary (updated)
-----------------
Send TASK_STARTING from the built-in executors.
Bugs: MESOS-7941
https://issues.apache.org/jira/browse/MESOS-7941
Repository: mesos
Description
-------
This gives schedulers more information about a tasks status,
in particular it gives a better estimate of a tasks start time
and helps differentiating between tasks stuck in TASK_STAGING
and tasks stuck in TASK_STARTING.
Diffs (updated)
-----
docs/high-availability-framework-guide.md 73743aba31f9d0ca827d318e2ecb4752a91b1be0
src/docker/executor.cpp e9949f652cd8527991ebfdfbf14e68b4c958fe79
src/launcher/default_executor.cpp 106b7f2e0244d211c66b237b5d1c51f43fc6e529
src/launcher/executor.cpp 951597b576b4912541dd87d52dcb981393e58082
Diff: https://reviews.apache.org/r/62212/diff/2/
Changes: https://reviews.apache.org/r/62212/diff/1-2/
Testing
-------
Thanks,
Benno Evers