Re: Review Request 43489: KillTask introduces TASK_KILLING state.

[Abhishek Dasgupta]

> On Feb. 18, 2016, 1:44 a.m., Qian Zhang wrote:
> > I see you send TASK_KILLING once the killTask() is invoked. However, 
> > according to the description of MESOS-4140, we may need to send such status 
> > update after SIGTERM is sent to the task and before SIGKILL is sent. So 
> > maybe you should send TASK_KILLING in shutdown() instead?
> 
> Abhishek Dasgupta wrote:
> Okay, i see. but in the case of docker executor, TASK_KILLING should come 
> after docker stop. Does it sound good??
> 
> Abhishek Dasgupta wrote:
> Actually, to set the status, we need to send the task_id also. Task_id is 
> not available inside shutdown. Moreover, I don't think it will make any 
> difference if it is called inside shutdown instead of killTask. It makes 
> sense to change the status a TASK_KILLING as soon as killTask is issued.
> 
> Qian Zhang wrote:
> shutdown() will be not only called by killTask(), but also called when 
> the executor itself is asked to shutdown, so if you send the status in 
> killTask(), that means for the latter case (executor shutdown), the 
> TASK_KILLING status will not be sent?
> 
> Abhishek Dasgupta wrote:
> So, here is the dubious scenario. If executor shuts down, you can't 
> exepect to provide task_id to shutdown() as there might be lots of tasks. 
> That's why the only argument of shutdown() is executordriver. But to update 
> TASK_KILLING status, you must need task_id. So, in my understanding, shutdown 
> by executor shut down should not require TASK_KILLING status as in that case, 
> all the tasks are unhealthy.
> 
> Ben Mahler wrote:
> Recall that executors can manage many tasks, the command/docker executors 
> are implementations provided by mesos and they manage a single task only. It 
> is up to the executor to decide how to shutdown, in the case of the command 
> executor we kill the active task and report TASK_KILLED (in both the killTask 
> and shutdown cases):
> 
> 
> https://github.com/apache/mesos/blob/0.27.0/src/launcher/executor.cpp#L514-L516
> 
> Since we'd like to send a TASK_KILLING when we start to kill a task, we 
> need to send it in both the killTask and shutdown.
> 
> Make sense?
> 
> Qian Zhang wrote:
> Make sense to me, but I see this patch has been submitted and 
> TASK_KILLING is only sent in killTask but not in shutdown?

The patch has been changed. Ben modified the patch to send TASK_KILLING inside 
shutdown.


- Abhishek


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


On Feb. 16, 2016, 10:23 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43489/
> ---
> 
> (Updated Feb. 16, 2016, 10:23 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Qian Zhang.
> 
> 
> Bugs: MESOS-4547
> https://issues.apache.org/jira/browse/MESOS-4547
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> KillTask introduces TASK_KILLING state.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 654a41db843a85d953880d5145bc95ada9ed2920 
>   src/launcher/executor.cpp c27e0792e4b9bcec0829a46be4232c013d965cf9 
> 
> Diff: https://reviews.apache.org/r/43489/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 43489: KillTask introduces TASK_KILLING state.

[Qian Zhang]

> On Feb. 21, 2016, 12:30 a.m., Ben Mahler wrote:
> > src/launcher/executor.cpp, lines 460-469
> > 
> >
> > Let's do this inside shutdown per my comment at the top.
> > 
> > It's also not clear to me why the health check process is only killed 
> > inside killTask, seems that we'll leak the health check process if we only 
> > are sent shutdown! Looks like a bug :(

Is it because the health check process is part of the executor process group 
and once executor is shutdown it will be terminated too?


- Qian


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


On Feb. 16, 2016, 6:23 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43489/
> ---
> 
> (Updated Feb. 16, 2016, 6:23 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Qian Zhang.
> 
> 
> Bugs: MESOS-4547
> https://issues.apache.org/jira/browse/MESOS-4547
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> KillTask introduces TASK_KILLING state.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 654a41db843a85d953880d5145bc95ada9ed2920 
>   src/launcher/executor.cpp c27e0792e4b9bcec0829a46be4232c013d965cf9 
> 
> Diff: https://reviews.apache.org/r/43489/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 43489: KillTask introduces TASK_KILLING state.

[Qian Zhang]

> On Feb. 18, 2016, 9:44 a.m., Qian Zhang wrote:
> > I see you send TASK_KILLING once the killTask() is invoked. However, 
> > according to the description of MESOS-4140, we may need to send such status 
> > update after SIGTERM is sent to the task and before SIGKILL is sent. So 
> > maybe you should send TASK_KILLING in shutdown() instead?
> 
> Abhishek Dasgupta wrote:
> Okay, i see. but in the case of docker executor, TASK_KILLING should come 
> after docker stop. Does it sound good??
> 
> Abhishek Dasgupta wrote:
> Actually, to set the status, we need to send the task_id also. Task_id is 
> not available inside shutdown. Moreover, I don't think it will make any 
> difference if it is called inside shutdown instead of killTask. It makes 
> sense to change the status a TASK_KILLING as soon as killTask is issued.
> 
> Qian Zhang wrote:
> shutdown() will be not only called by killTask(), but also called when 
> the executor itself is asked to shutdown, so if you send the status in 
> killTask(), that means for the latter case (executor shutdown), the 
> TASK_KILLING status will not be sent?
> 
> Abhishek Dasgupta wrote:
> So, here is the dubious scenario. If executor shuts down, you can't 
> exepect to provide task_id to shutdown() as there might be lots of tasks. 
> That's why the only argument of shutdown() is executordriver. But to update 
> TASK_KILLING status, you must need task_id. So, in my understanding, shutdown 
> by executor shut down should not require TASK_KILLING status as in that case, 
> all the tasks are unhealthy.
> 
> Ben Mahler wrote:
> Recall that executors can manage many tasks, the command/docker executors 
> are implementations provided by mesos and they manage a single task only. It 
> is up to the executor to decide how to shutdown, in the case of the command 
> executor we kill the active task and report TASK_KILLED (in both the killTask 
> and shutdown cases):
> 
> 
> https://github.com/apache/mesos/blob/0.27.0/src/launcher/executor.cpp#L514-L516
> 
> Since we'd like to send a TASK_KILLING when we start to kill a task, we 
> need to send it in both the killTask and shutdown.
> 
> Make sense?

Make sense to me, but I see this patch has been submitted and TASK_KILLING is 
only sent in killTask but not in shutdown?


- Qian


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


On Feb. 16, 2016, 6:23 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43489/
> ---
> 
> (Updated Feb. 16, 2016, 6:23 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Qian Zhang.
> 
> 
> Bugs: MESOS-4547
> https://issues.apache.org/jira/browse/MESOS-4547
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> KillTask introduces TASK_KILLING state.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 654a41db843a85d953880d5145bc95ada9ed2920 
>   src/launcher/executor.cpp c27e0792e4b9bcec0829a46be4232c013d965cf9 
> 
> Diff: https://reviews.apache.org/r/43489/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 43489: KillTask introduces TASK_KILLING state.

[Ben Mahler]

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



It would be helpful to reference in the testing done section that you've also 
added tests in subsequent patches, so that it's clear to the reviewer without 
having to look through the entire chain. I left some comments but I'll make the 
adjustments since they're minor.


src/docker/executor.cpp (lines 209 - 212)


Ditto here, I can't see why this is only done inside killTask, looks like a 
bug!



src/launcher/executor.cpp (line 117)


To be consistent, let's set frameworkInfo right below where we set driver.



src/launcher/executor.cpp (lines 460 - 469)


Let's do this inside shutdown per my comment at the top.

It's also not clear to me why the health check process is only killed 
inside killTask, seems that we'll leak the health check process if we only are 
sent shutdown! Looks like a bug :(



src/launcher/executor.cpp (lines 460 - 461)


Let's avoid copying every capability we loop over (take a const & instead), 
since Capability may store additional data in the future and we don't need to 
mutate a copy here.



src/launcher/executor.cpp (line 461)


Please add a CHECK_SOME to ensure that frameworkInfo is Some before you 
de-reference.

Also, you can use the -> operator to clean this up:

```
foreach (FrameworkInfo::Capability capability,
 frameworkInfo->capabilities()) {
```

A space is needed before the brace on the foreach: s/){/) {/



src/launcher/executor.cpp (line 464)


Please use CopyFrom for copying protobufs, unless you need the merge 
functionality (you don't need merging functionality here).


- Ben Mahler


On Feb. 16, 2016, 10:23 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43489/
> ---
> 
> (Updated Feb. 16, 2016, 10:23 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Qian Zhang.
> 
> 
> Bugs: MESOS-4547
> https://issues.apache.org/jira/browse/MESOS-4547
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> KillTask introduces TASK_KILLING state.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 654a41db843a85d953880d5145bc95ada9ed2920 
>   src/launcher/executor.cpp c27e0792e4b9bcec0829a46be4232c013d965cf9 
> 
> Diff: https://reviews.apache.org/r/43489/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 43489: KillTask introduces TASK_KILLING state.

[Ben Mahler]

> On Feb. 18, 2016, 1:44 a.m., Qian Zhang wrote:
> > I see you send TASK_KILLING once the killTask() is invoked. However, 
> > according to the description of MESOS-4140, we may need to send such status 
> > update after SIGTERM is sent to the task and before SIGKILL is sent. So 
> > maybe you should send TASK_KILLING in shutdown() instead?
> 
> Abhishek Dasgupta wrote:
> Okay, i see. but in the case of docker executor, TASK_KILLING should come 
> after docker stop. Does it sound good??
> 
> Abhishek Dasgupta wrote:
> Actually, to set the status, we need to send the task_id also. Task_id is 
> not available inside shutdown. Moreover, I don't think it will make any 
> difference if it is called inside shutdown instead of killTask. It makes 
> sense to change the status a TASK_KILLING as soon as killTask is issued.
> 
> Qian Zhang wrote:
> shutdown() will be not only called by killTask(), but also called when 
> the executor itself is asked to shutdown, so if you send the status in 
> killTask(), that means for the latter case (executor shutdown), the 
> TASK_KILLING status will not be sent?
> 
> Abhishek Dasgupta wrote:
> So, here is the dubious scenario. If executor shuts down, you can't 
> exepect to provide task_id to shutdown() as there might be lots of tasks. 
> That's why the only argument of shutdown() is executordriver. But to update 
> TASK_KILLING status, you must need task_id. So, in my understanding, shutdown 
> by executor shut down should not require TASK_KILLING status as in that case, 
> all the tasks are unhealthy.

Recall that executors can manage many tasks, the command/docker executors are 
implementations provided by mesos and they manage a single task only. It is up 
to the executor to decide how to shutdown, in the case of the command executor 
we kill the active task and report TASK_KILLED (in both the killTask and 
shutdown cases):

https://github.com/apache/mesos/blob/0.27.0/src/launcher/executor.cpp#L514-L516

Since we'd like to send a TASK_KILLING when we start to kill a task, we need to 
send it in both the killTask and shutdown.

Make sense?


- Ben


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


On Feb. 16, 2016, 10:23 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43489/
> ---
> 
> (Updated Feb. 16, 2016, 10:23 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Qian Zhang.
> 
> 
> Bugs: MESOS-4547
> https://issues.apache.org/jira/browse/MESOS-4547
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> KillTask introduces TASK_KILLING state.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 654a41db843a85d953880d5145bc95ada9ed2920 
>   src/launcher/executor.cpp c27e0792e4b9bcec0829a46be4232c013d965cf9 
> 
> Diff: https://reviews.apache.org/r/43489/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 43489: KillTask introduces TASK_KILLING state.

[Abhishek Dasgupta]

> On Feb. 18, 2016, 1:44 a.m., Qian Zhang wrote:
> > I see you send TASK_KILLING once the killTask() is invoked. However, 
> > according to the description of MESOS-4140, we may need to send such status 
> > update after SIGTERM is sent to the task and before SIGKILL is sent. So 
> > maybe you should send TASK_KILLING in shutdown() instead?
> 
> Abhishek Dasgupta wrote:
> Okay, i see. but in the case of docker executor, TASK_KILLING should come 
> after docker stop. Does it sound good??
> 
> Abhishek Dasgupta wrote:
> Actually, to set the status, we need to send the task_id also. Task_id is 
> not available inside shutdown. Moreover, I don't think it will make any 
> difference if it is called inside shutdown instead of killTask. It makes 
> sense to change the status a TASK_KILLING as soon as killTask is issued.
> 
> Qian Zhang wrote:
> shutdown() will be not only called by killTask(), but also called when 
> the executor itself is asked to shutdown, so if you send the status in 
> killTask(), that means for the latter case (executor shutdown), the 
> TASK_KILLING status will not be sent?

So, here is the dubious scenario. If executor shuts down, you can't exepect to 
provide task_id to shutdown() as there might be lots of tasks. That's why the 
only argument of shutdown() is executordriver. But to update TASK_KILLING 
status, you must need task_id. So, in my understanding, shutdown by executor 
shut down should not require TASK_KILLING status as in that case, all the tasks 
are unhealthy.


- Abhishek


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


On Feb. 16, 2016, 10:23 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43489/
> ---
> 
> (Updated Feb. 16, 2016, 10:23 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Qian Zhang.
> 
> 
> Bugs: MESOS-4547
> https://issues.apache.org/jira/browse/MESOS-4547
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> KillTask introduces TASK_KILLING state.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 654a41db843a85d953880d5145bc95ada9ed2920 
>   src/launcher/executor.cpp c27e0792e4b9bcec0829a46be4232c013d965cf9 
> 
> Diff: https://reviews.apache.org/r/43489/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 43489: KillTask introduces TASK_KILLING state.

[Qian Zhang]

> On Feb. 18, 2016, 9:44 a.m., Qian Zhang wrote:
> > I see you send TASK_KILLING once the killTask() is invoked. However, 
> > according to the description of MESOS-4140, we may need to send such status 
> > update after SIGTERM is sent to the task and before SIGKILL is sent. So 
> > maybe you should send TASK_KILLING in shutdown() instead?
> 
> Abhishek Dasgupta wrote:
> Okay, i see. but in the case of docker executor, TASK_KILLING should come 
> after docker stop. Does it sound good??
> 
> Abhishek Dasgupta wrote:
> Actually, to set the status, we need to send the task_id also. Task_id is 
> not available inside shutdown. Moreover, I don't think it will make any 
> difference if it is called inside shutdown instead of killTask. It makes 
> sense to change the status a TASK_KILLING as soon as killTask is issued.

shutdown() will be not only called by killTask(), but also called when the 
executor itself is asked to shutdown, so if you send the status in killTask(), 
that means for the latter case (executor shutdown), the TASK_KILLING status 
will not be sent?


- Qian


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


On Feb. 16, 2016, 6:23 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43489/
> ---
> 
> (Updated Feb. 16, 2016, 6:23 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Qian Zhang.
> 
> 
> Bugs: MESOS-4547
> https://issues.apache.org/jira/browse/MESOS-4547
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> KillTask introduces TASK_KILLING state.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 654a41db843a85d953880d5145bc95ada9ed2920 
>   src/launcher/executor.cpp c27e0792e4b9bcec0829a46be4232c013d965cf9 
> 
> Diff: https://reviews.apache.org/r/43489/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 43489: KillTask introduces TASK_KILLING state.

[Abhishek Dasgupta]

> On Feb. 18, 2016, 1:44 a.m., Qian Zhang wrote:
> > I see you send TASK_KILLING once the killTask() is invoked. However, 
> > according to the description of MESOS-4140, we may need to send such status 
> > update after SIGTERM is sent to the task and before SIGKILL is sent. So 
> > maybe you should send TASK_KILLING in shutdown() instead?
> 
> Abhishek Dasgupta wrote:
> Okay, i see. but in the case of docker executor, TASK_KILLING should come 
> after docker stop. Does it sound good??

Actually, to set the status, we need to send the task_id also. Task_id is not 
available inside shutdown. Moreover, I don't think it will make any difference 
if it is called inside shutdown instead of killTask. It makes sense to change 
the status a TASK_KILLING as soon as killTask is issued.


- Abhishek


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


On Feb. 16, 2016, 10:23 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43489/
> ---
> 
> (Updated Feb. 16, 2016, 10:23 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Qian Zhang.
> 
> 
> Bugs: MESOS-4547
> https://issues.apache.org/jira/browse/MESOS-4547
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> KillTask introduces TASK_KILLING state.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 654a41db843a85d953880d5145bc95ada9ed2920 
>   src/launcher/executor.cpp c27e0792e4b9bcec0829a46be4232c013d965cf9 
> 
> Diff: https://reviews.apache.org/r/43489/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 43489: KillTask introduces TASK_KILLING state.

[Abhishek Dasgupta]

> On Feb. 18, 2016, 1:44 a.m., Qian Zhang wrote:
> > I see you send TASK_KILLING once the killTask() is invoked. However, 
> > according to the description of MESOS-4140, we may need to send such status 
> > update after SIGTERM is sent to the task and before SIGKILL is sent. So 
> > maybe you should send TASK_KILLING in shutdown() instead?

Okay, i see. but in the case of docker executor, TASK_KILLING should come after 
docker stop. Does it sound good??


- Abhishek


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


On Feb. 16, 2016, 10:23 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43489/
> ---
> 
> (Updated Feb. 16, 2016, 10:23 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Qian Zhang.
> 
> 
> Bugs: MESOS-4547
> https://issues.apache.org/jira/browse/MESOS-4547
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> KillTask introduces TASK_KILLING state.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 654a41db843a85d953880d5145bc95ada9ed2920 
>   src/launcher/executor.cpp c27e0792e4b9bcec0829a46be4232c013d965cf9 
> 
> Diff: https://reviews.apache.org/r/43489/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 43489: KillTask introduces TASK_KILLING state.

[Qian Zhang]

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



I see you send TASK_KILLING once the killTask() is invoked. However, according 
to the description of MESOS-4140, we may need to send such status update after 
SIGTERM is sent to the task and before SIGKILL is sent. So maybe you should 
send TASK_KILLING in shutdown() instead?

- Qian Zhang


On Feb. 16, 2016, 6:23 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43489/
> ---
> 
> (Updated Feb. 16, 2016, 6:23 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Qian Zhang.
> 
> 
> Bugs: MESOS-4547
> https://issues.apache.org/jira/browse/MESOS-4547
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> KillTask introduces TASK_KILLING state.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 654a41db843a85d953880d5145bc95ada9ed2920 
>   src/launcher/executor.cpp c27e0792e4b9bcec0829a46be4232c013d965cf9 
> 
> Diff: https://reviews.apache.org/r/43489/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 43489: KillTask introduces TASK_KILLING state.

[Abhishek Dasgupta]

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

(Updated Feb. 16, 2016, 10:23 a.m.)


Review request for mesos, Ben Mahler and Qian Zhang.


Bugs: MESOS-4547
https://issues.apache.org/jira/browse/MESOS-4547


Repository: mesos


Description
---

KillTask introduces TASK_KILLING state.


Diffs (updated)
-

  src/docker/executor.cpp 654a41db843a85d953880d5145bc95ada9ed2920 
  src/launcher/executor.cpp c27e0792e4b9bcec0829a46be4232c013d965cf9 

Diff: https://reviews.apache.org/r/43489/diff/


Testing
---

make check


Thanks,

Abhishek Dasgupta

Re: Review Request 43489: KillTask introduces TASK_KILLING state.

[Guangya Liu]

> On 二月 16, 2016, 9:06 a.m., Guangya Liu wrote:
> > src/launcher/executor.cpp, line 461
> > 
> >
> > adjust the alignment: Keep 4 space align with `foreach`

Sorry, not 4 spaces, but keep align with above `FrameworkInfo`

foreach (FrameworkInfo::Capability capability,
 frameworkInfo.get().capabilities()){


> On 二月 16, 2016, 9:06 a.m., Guangya Liu wrote:
> > src/docker/executor.cpp, line 199
> > 
> >
> > adjust the alignment: Keep 4 space align with `foreach`

Sorry, not 4 spaces, but keep align with above `FrameworkInfo`

foreach (FrameworkInfo::Capability capability,
 frameworkInfo.get().capabilities()){


- Guangya


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


On 二月 16, 2016, 8:28 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43489/
> ---
> 
> (Updated 二月 16, 2016, 8:28 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Qian Zhang.
> 
> 
> Bugs: MESOS-4547
> https://issues.apache.org/jira/browse/MESOS-4547
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> KillTask introduces TASK_KILLING state.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 654a41db843a85d953880d5145bc95ada9ed2920 
>   src/launcher/executor.cpp c27e0792e4b9bcec0829a46be4232c013d965cf9 
> 
> Diff: https://reviews.apache.org/r/43489/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 43489: KillTask introduces TASK_KILLING state.

[Guangya Liu]

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




src/docker/executor.cpp (line 199)


adjust the alignment: Keep 4 space align with `foreach`



src/launcher/executor.cpp (line 461)


adjust the alignment: Keep 4 space align with `foreach`



src/launcher/executor.cpp (line 465)


s/TASK_RUNNING/TASK_KILLING


- Guangya Liu


On 二月 16, 2016, 8:28 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43489/
> ---
> 
> (Updated 二月 16, 2016, 8:28 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Qian Zhang.
> 
> 
> Bugs: MESOS-4547
> https://issues.apache.org/jira/browse/MESOS-4547
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> KillTask introduces TASK_KILLING state.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 654a41db843a85d953880d5145bc95ada9ed2920 
>   src/launcher/executor.cpp c27e0792e4b9bcec0829a46be4232c013d965cf9 
> 
> Diff: https://reviews.apache.org/r/43489/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 43489: KillTask introduces TASK_KILLING state.

[Jian Qiu]

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




src/docker/executor.cpp (line 203)


the state should be TASK_KILLING?


- Jian Qiu


On 二月 16, 2016, 8:28 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43489/
> ---
> 
> (Updated 二月 16, 2016, 8:28 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Qian Zhang.
> 
> 
> Bugs: MESOS-4547
> https://issues.apache.org/jira/browse/MESOS-4547
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> KillTask introduces TASK_KILLING state.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 654a41db843a85d953880d5145bc95ada9ed2920 
>   src/launcher/executor.cpp c27e0792e4b9bcec0829a46be4232c013d965cf9 
> 
> Diff: https://reviews.apache.org/r/43489/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 43489: KillTask introduces TASK_KILLING state.

[Abhishek Dasgupta]

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

(Updated Feb. 16, 2016, 8:28 a.m.)


Review request for mesos, Ben Mahler and Qian Zhang.


Bugs: MESOS-4547
https://issues.apache.org/jira/browse/MESOS-4547


Repository: mesos


Description
---

KillTask introduces TASK_KILLING state.


Diffs (updated)
-

  src/docker/executor.cpp 654a41db843a85d953880d5145bc95ada9ed2920 
  src/launcher/executor.cpp c27e0792e4b9bcec0829a46be4232c013d965cf9 

Diff: https://reviews.apache.org/r/43489/diff/


Testing
---

make check


Thanks,

Abhishek Dasgupta