You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Anand Mazumdar <an...@apache.org> on 2017/02/20 05:00:30 UTC

Review Request 56830: Introduced `TaskInfo::OnTerminationPolicy` protobuf.

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

Review request for mesos, Vinod Kone and Jiang Yan Xu.


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


Repository: mesos


Description
-------

Describes the on termination policy associated with a task and is
applied upon a task termination.


Diffs
-----

  include/mesos/mesos.proto 030e79c003f6560e9c0627db12fb1baba411151d 
  include/mesos/v1/mesos.proto 7f6f858a7d9387d202510730d400e490298e6574 

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


Testing
-------

make check (tests later in the chain)


Thanks,

Anand Mazumdar

Re: Review Request 56830: Introduced `TaskInfo::OnTerminationPolicy` protobuf.

Posted by Jiang Yan Xu <ya...@jxu.me>.

> On Feb. 21, 2017, 1:50 p.m., Vinod Kone wrote:
> > include/mesos/mesos.proto, line 1581
> > <https://reviews.apache.org/r/56830/diff/1/?file=1640428#file1640428line1581>
> >
> >     also for unknown status (e.g., OOM)

Since Mesos never defined tasks as processes only, It's whatever the executor decides, I think it's fine to not define it directly here but rather refer to `TASK_FAILED`. i.e., however TASK_FAILED is defined.

Would this be clear:

```
// The task group would be only killed when the task failed,
// i.e., it will be transitioned to TASK_FAILED state.
```


- Jiang Yan


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


On Feb. 19, 2017, 9 p.m., Anand Mazumdar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56830/
> -----------------------------------------------------------
> 
> (Updated Feb. 19, 2017, 9 p.m.)
> 
> 
> Review request for mesos, Vinod Kone and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7067
>     https://issues.apache.org/jira/browse/MESOS-7067
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Describes the on termination policy associated with a task and is
> applied upon a task termination.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 030e79c003f6560e9c0627db12fb1baba411151d 
>   include/mesos/v1/mesos.proto 7f6f858a7d9387d202510730d400e490298e6574 
> 
> Diff: https://reviews.apache.org/r/56830/diff/
> 
> 
> Testing
> -------
> 
> make check (tests later in the chain)
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 56830: Introduced `TaskInfo::OnTerminationPolicy` protobuf.

Posted by Anand Mazumdar <an...@apache.org>.

> On Feb. 21, 2017, 9:50 p.m., Vinod Kone wrote:
> > include/mesos/mesos.proto, line 1581
> > <https://reviews.apache.org/r/56830/diff/1/?file=1640428#file1640428line1581>
> >
> >     also for unknown status (e.g., OOM)
> 
> Jiang Yan Xu wrote:
>     Since Mesos never defined tasks as processes only, It's whatever the executor decides, I think it's fine to not define it directly here but rather refer to `TASK_FAILED`. i.e., however TASK_FAILED is defined.
>     
>     Would this be clear:
>     
>     ```
>     // The task group would be only killed when the task failed,
>     // i.e., it will be transitioned to TASK_FAILED state.
>     ```

Fair enough. Clarified comments specifically for the default executor case.


> On Feb. 21, 2017, 9:50 p.m., Vinod Kone wrote:
> > include/mesos/mesos.proto, line 1558
> > <https://reviews.apache.org/r/56830/diff/1/?file=1640428#file1640428line1558>
> >
> >     s/On//
> 
> Jiang Yan Xu wrote:
>     I think `OnTerminationPolicy` looks ugly not because of `On` but rather it's a three word term which is a bit mouthful. `OnTermination` or `OnTerminate` by itself is widely used in similar contexts. I started to wonder if the word `Policy` add much additional info. The protobuf is clearly about how to **handle** (i.e., **action** to take on) termination. Maybe none of these words are necessary?

hmm, `OnTermination` and `OnTerminate` look more like handler names and hence not quite suitable. I think it's fine to use `TerminationPolicy` here while being explicit in the comments to dissambiguate it.


- Anand


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


On Feb. 20, 2017, 5 a.m., Anand Mazumdar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56830/
> -----------------------------------------------------------
> 
> (Updated Feb. 20, 2017, 5 a.m.)
> 
> 
> Review request for mesos, Vinod Kone and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7067
>     https://issues.apache.org/jira/browse/MESOS-7067
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Describes the on termination policy associated with a task and is
> applied upon a task termination.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 030e79c003f6560e9c0627db12fb1baba411151d 
>   include/mesos/v1/mesos.proto 7f6f858a7d9387d202510730d400e490298e6574 
> 
> Diff: https://reviews.apache.org/r/56830/diff/
> 
> 
> Testing
> -------
> 
> make check (tests later in the chain)
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 56830: Introduced `TaskInfo::OnTerminationPolicy` protobuf.

Posted by Jiang Yan Xu <ya...@jxu.me>.

> On Feb. 21, 2017, 1:50 p.m., Vinod Kone wrote:
> > include/mesos/mesos.proto, line 1558
> > <https://reviews.apache.org/r/56830/diff/1/?file=1640428#file1640428line1558>
> >
> >     s/On//

I think `OnTerminationPolicy` looks ugly not because of `On` but rather it's a three word term which is a bit mouthful. `OnTermination` or `OnTerminate` by itself is widely used in similar contexts. I started to wonder if the word `Policy` add much additional info. The protobuf is clearly about how to **handle** (i.e., **action** to take on) termination. Maybe none of these words are necessary?


- Jiang Yan


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


On Feb. 19, 2017, 9 p.m., Anand Mazumdar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56830/
> -----------------------------------------------------------
> 
> (Updated Feb. 19, 2017, 9 p.m.)
> 
> 
> Review request for mesos, Vinod Kone and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7067
>     https://issues.apache.org/jira/browse/MESOS-7067
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Describes the on termination policy associated with a task and is
> applied upon a task termination.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 030e79c003f6560e9c0627db12fb1baba411151d 
>   include/mesos/v1/mesos.proto 7f6f858a7d9387d202510730d400e490298e6574 
> 
> Diff: https://reviews.apache.org/r/56830/diff/
> 
> 
> Testing
> -------
> 
> make check (tests later in the chain)
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 56830: Introduced `TaskInfo::OnTerminationPolicy` protobuf.

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56830/#review166233
-----------------------------------------------------------




include/mesos/mesos.proto (line 1558)
<https://reviews.apache.org/r/56830/#comment238134>

    s/On//



include/mesos/mesos.proto (line 1567)
<https://reviews.apache.org/r/56830/#comment238135>

    s/KILL_TASK_GROUP/KILL_GROUP/



include/mesos/mesos.proto (line 1571)
<https://reviews.apache.org/r/56830/#comment238136>

    s/KillTaskGroup/KillGroup/



include/mesos/mesos.proto (line 1581)
<https://reviews.apache.org/r/56830/#comment238137>

    also for unknown status (e.g., OOM)


- Vinod Kone


On Feb. 20, 2017, 5 a.m., Anand Mazumdar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56830/
> -----------------------------------------------------------
> 
> (Updated Feb. 20, 2017, 5 a.m.)
> 
> 
> Review request for mesos, Vinod Kone and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7067
>     https://issues.apache.org/jira/browse/MESOS-7067
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Describes the on termination policy associated with a task and is
> applied upon a task termination.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 030e79c003f6560e9c0627db12fb1baba411151d 
>   include/mesos/v1/mesos.proto 7f6f858a7d9387d202510730d400e490298e6574 
> 
> Diff: https://reviews.apache.org/r/56830/diff/
> 
> 
> Testing
> -------
> 
> make check (tests later in the chain)
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 56830: Introduced `TaskInfo::OnTerminationPolicy` protobuf.

Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56830/#review166255
-----------------------------------------------------------




include/mesos/mesos.proto (lines 1641 - 1642)
<https://reviews.apache.org/r/56830/#comment238169>

    I think for our protos we pretty much follow the rule to define the nested message right above where it's used (If its use is singular). e.g., Resource. Any reason to define it at the top?


- Jiang Yan Xu


On Feb. 19, 2017, 9 p.m., Anand Mazumdar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56830/
> -----------------------------------------------------------
> 
> (Updated Feb. 19, 2017, 9 p.m.)
> 
> 
> Review request for mesos, Vinod Kone and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7067
>     https://issues.apache.org/jira/browse/MESOS-7067
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Describes the on termination policy associated with a task and is
> applied upon a task termination.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 030e79c003f6560e9c0627db12fb1baba411151d 
>   include/mesos/v1/mesos.proto 7f6f858a7d9387d202510730d400e490298e6574 
> 
> Diff: https://reviews.apache.org/r/56830/diff/
> 
> 
> Testing
> -------
> 
> make check (tests later in the chain)
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 56830: Introduced `TaskInfo::TerminationPolicy` protobuf.

Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56830/#review166388
-----------------------------------------------------------




include/mesos/mesos.proto (line 1603)
<https://reviews.apache.org/r/56830/#comment238313>

    It isn't clear that "task termination" does not include "when the task is explicitly killed by the scheduler." I'd say something like:
    
    "Describes the policy that will be used when a task terminates. This policy is only used when a task terminates (i.e., finishes successfully or exits with an error); it is not used for tasks that are explicitly killed by the scheduler."
    
    You should also explain whether the policy is used when tasks are killed by the agent (e.g., due to oversubscription or because of a containerizer error).



include/mesos/mesos.proto (line 1611)
<https://reviews.apache.org/r/56830/#comment238314>

    "it is"



include/mesos/mesos.proto (line 1614)
<https://reviews.apache.org/r/56830/#comment238315>

    `IGNORE` would be an alternative to `DO_NOTHING`; I don't have a strong view on which term is better.



include/mesos/mesos.proto (line 1623)
<https://reviews.apache.org/r/56830/#comment238316>

    "The task group will always be killed, regardless of whether the task finished successfully or failed -- i.e., transitioned to the TASK_FINISHED or TASK_FAILED states, respectively."
    
    Last sentence doesn't make sense to me -- i.e., "zero exit code or failed" covers all cases, right?



include/mesos/mesos.proto (line 1629)
<https://reviews.apache.org/r/56830/#comment238317>

    "The task group will only be killed when the task fails -- i.e., transitions to the TASK_FAILED state. For the default executor, this happens when the task exits with a non-zero exit code."
    
    Note that OOM killer (i.e., SIGKILL) does _not_ result in the exit code being "unknown" -- see http://unix.stackexchange.com/a/99134


- Neil Conway


On Feb. 22, 2017, 6:16 p.m., Anand Mazumdar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56830/
> -----------------------------------------------------------
> 
> (Updated Feb. 22, 2017, 6:16 p.m.)
> 
> 
> Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7067
>     https://issues.apache.org/jira/browse/MESOS-7067
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Describes the termination policy associated with a task and is
> applied upon a task termination.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 030e79c003f6560e9c0627db12fb1baba411151d 
>   include/mesos/v1/mesos.proto 7f6f858a7d9387d202510730d400e490298e6574 
> 
> Diff: https://reviews.apache.org/r/56830/diff/
> 
> 
> Testing
> -------
> 
> make check (tests later in the chain)
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 56830: [WIP] Introduced `TaskInfo::TerminationPolicy` protobuf.

Posted by Anand Mazumdar <an...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56830/
-----------------------------------------------------------

(Updated Feb. 23, 2017, 12:09 a.m.)


Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.


Changes
-------

Review comments from Neil. Changing the summary to be WIP as there were a few offline discussions between me and Vinod that might result in further changes
to the way the protobuf is structured.


Summary (updated)
-----------------

[WIP] Introduced `TaskInfo::TerminationPolicy` protobuf.


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


Repository: mesos


Description
-------

Describes the termination policy associated with a task and is
applied upon a task termination.


Diffs (updated)
-----

  include/mesos/mesos.proto 030e79c003f6560e9c0627db12fb1baba411151d 
  include/mesos/v1/mesos.proto 7f6f858a7d9387d202510730d400e490298e6574 

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


Testing
-------

make check (tests later in the chain)


Thanks,

Anand Mazumdar

Re: Review Request 56830: Introduced `TaskInfo::TerminationPolicy` protobuf.

Posted by Anand Mazumdar <an...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56830/
-----------------------------------------------------------

(Updated Feb. 22, 2017, 6:16 p.m.)


Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.


Changes
-------

Review comments + Modified `PROCEED` action to `DO_NOTHING` since it better signifies the intent based on offline discussions.

+ Adding Neil as a reviewer too.


Summary (updated)
-----------------

Introduced `TaskInfo::TerminationPolicy` protobuf.


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


Repository: mesos


Description (updated)
-------

Describes the termination policy associated with a task and is
applied upon a task termination.


Diffs (updated)
-----

  include/mesos/mesos.proto 030e79c003f6560e9c0627db12fb1baba411151d 
  include/mesos/v1/mesos.proto 7f6f858a7d9387d202510730d400e490298e6574 

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


Testing
-------

make check (tests later in the chain)


Thanks,

Anand Mazumdar