You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Greg Mann <gr...@mesosphere.io> on 2019/07/08 18:25:16 UTC

Review Request 71034: Enabled the Docker executor to accept kill policy overrides.

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

Review request for mesos, Benno Evers, Benjamin Mahler, and Joseph Wu.


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


Repository: mesos


Description
-------

This adds a new `killTask()` overload to the Docker executor
and updates the Mesos executor driver to call into that
overload when the loaded executor is the Docker executor.

This allows the executor driver to pass the kill policy
override, when present, into the Docker executor.


Diffs
-----

  src/docker/executor.hpp f21e84c71f646e84404c65fc2ded64bcaff482ef 
  src/docker/executor.cpp f638e4b65155bcca1be36424b7061ea26a3d6ca3 
  src/exec/exec.cpp c0fa3b61667da96bc4395bae9956c54446268122 


Diff: https://reviews.apache.org/r/71034/diff/1/


Testing
-------

Details at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 71034: Enabled the Docker executor to accept kill policy overrides.

Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71034/#review216621
-----------------------------------------------------------


Fix it, then Ship it!





src/docker/executor.cpp
Lines 985-987 (patched)
<https://reviews.apache.org/r/71034/#comment303835>

    Nit: Add `// Need to disambiguate overloaded function.`


- Joseph Wu


On July 10, 2019, 1:17 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71034/
> -----------------------------------------------------------
> 
> (Updated July 10, 2019, 1:17 p.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, and Joseph Wu.
> 
> 
> Bugs: MESOS-9853
>     https://issues.apache.org/jira/browse/MESOS-9853
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This adds a new `killTask()` overload to the Docker executor
> and updates the Mesos executor driver to call into that
> overload when the loaded executor is the Docker executor.
> 
> This allows the executor driver to pass the kill policy
> override, when present, into the Docker executor.
> 
> 
> Diffs
> -----
> 
>   src/docker/executor.hpp f21e84c71f646e84404c65fc2ded64bcaff482ef 
>   src/docker/executor.cpp f638e4b65155bcca1be36424b7061ea26a3d6ca3 
>   src/exec/exec.cpp c0fa3b61667da96bc4395bae9956c54446268122 
> 
> 
> Diff: https://reviews.apache.org/r/71034/diff/2/
> 
> 
> Testing
> -------
> 
> Details at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 71034: Enabled the Docker executor to accept kill policy overrides.

Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71034/
-----------------------------------------------------------

(Updated July 25, 2019, 7:24 p.m.)


Review request for mesos, Benno Evers, Benjamin Mahler, and Joseph Wu.


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


Repository: mesos


Description
-------

This adds a new `killTask()` overload to the Docker executor
and updates the Mesos executor driver to call into that
overload when the loaded executor is the Docker executor.

This allows the executor driver to pass the kill policy
override, when present, into the Docker executor.


Diffs (updated)
-----

  src/docker/executor.hpp f21e84c71f646e84404c65fc2ded64bcaff482ef 
  src/docker/executor.cpp f638e4b65155bcca1be36424b7061ea26a3d6ca3 
  src/exec/exec.cpp c0fa3b61667da96bc4395bae9956c54446268122 


Diff: https://reviews.apache.org/r/71034/diff/3/

Changes: https://reviews.apache.org/r/71034/diff/2-3/


Testing
-------

Details at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 71034: Enabled the Docker executor to accept kill policy overrides.

Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71034/
-----------------------------------------------------------

(Updated July 10, 2019, 8:17 p.m.)


Review request for mesos, Benno Evers, Benjamin Mahler, and Joseph Wu.


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


Repository: mesos


Description
-------

This adds a new `killTask()` overload to the Docker executor
and updates the Mesos executor driver to call into that
overload when the loaded executor is the Docker executor.

This allows the executor driver to pass the kill policy
override, when present, into the Docker executor.


Diffs (updated)
-----

  src/docker/executor.hpp f21e84c71f646e84404c65fc2ded64bcaff482ef 
  src/docker/executor.cpp f638e4b65155bcca1be36424b7061ea26a3d6ca3 
  src/exec/exec.cpp c0fa3b61667da96bc4395bae9956c54446268122 


Diff: https://reviews.apache.org/r/71034/diff/2/

Changes: https://reviews.apache.org/r/71034/diff/1-2/


Testing
-------

Details at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 71034: Enabled the Docker executor to accept kill policy overrides.

Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71034/#review216430
-----------------------------------------------------------




src/docker/executor.hpp
Lines 157 (patched)
<https://reviews.apache.org/r/71034/#comment303658>

    Missing after the type `const Option<KillPolicy>`, an `&`



src/docker/executor.cpp
Line 932 (original), 938-950 (patched)
<https://reviews.apache.org/r/71034/#comment303657>

    Can you try disambiguating like this:
    ```
    // Need to disambiguate overloaded function.
    void (DockerExecutorProcess::*killTask)(
        ExecutorDriver*, const TaskID&, const Option<KillPolicy>&)
      = &DockerExecutorProcess::killTask;
    
    return process::dispatch(
      process.get(), killTask, driver, taskId, None());
    ```



src/docker/executor.cpp
Lines 980 (patched)
<https://reviews.apache.org/r/71034/#comment303659>

    Missing `&` after the Option<>.


- Joseph Wu


On July 8, 2019, 11:25 a.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71034/
> -----------------------------------------------------------
> 
> (Updated July 8, 2019, 11:25 a.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, and Joseph Wu.
> 
> 
> Bugs: MESOS-9853
>     https://issues.apache.org/jira/browse/MESOS-9853
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This adds a new `killTask()` overload to the Docker executor
> and updates the Mesos executor driver to call into that
> overload when the loaded executor is the Docker executor.
> 
> This allows the executor driver to pass the kill policy
> override, when present, into the Docker executor.
> 
> 
> Diffs
> -----
> 
>   src/docker/executor.hpp f21e84c71f646e84404c65fc2ded64bcaff482ef 
>   src/docker/executor.cpp f638e4b65155bcca1be36424b7061ea26a3d6ca3 
>   src/exec/exec.cpp c0fa3b61667da96bc4395bae9956c54446268122 
> 
> 
> Diff: https://reviews.apache.org/r/71034/diff/1/
> 
> 
> Testing
> -------
> 
> Details at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 71034: Enabled the Docker executor to accept kill policy overrides.

Posted by Greg Mann <gr...@mesosphere.io>.

> On July 10, 2019, 3:19 p.m., Benno Evers wrote:
> > src/docker/executor.cpp
> > Line 401 (original), 404 (patched)
> > <https://reviews.apache.org/r/71034/diff/1/?file=2153973#file2153973line404>
> >
> >     Should we print the override value here if it is provided?

Good call; done.


- Greg


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


On July 10, 2019, 8:17 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71034/
> -----------------------------------------------------------
> 
> (Updated July 10, 2019, 8:17 p.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, and Joseph Wu.
> 
> 
> Bugs: MESOS-9853
>     https://issues.apache.org/jira/browse/MESOS-9853
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This adds a new `killTask()` overload to the Docker executor
> and updates the Mesos executor driver to call into that
> overload when the loaded executor is the Docker executor.
> 
> This allows the executor driver to pass the kill policy
> override, when present, into the Docker executor.
> 
> 
> Diffs
> -----
> 
>   src/docker/executor.hpp f21e84c71f646e84404c65fc2ded64bcaff482ef 
>   src/docker/executor.cpp f638e4b65155bcca1be36424b7061ea26a3d6ca3 
>   src/exec/exec.cpp c0fa3b61667da96bc4395bae9956c54446268122 
> 
> 
> Diff: https://reviews.apache.org/r/71034/diff/2/
> 
> 
> Testing
> -------
> 
> Details at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 71034: Enabled the Docker executor to accept kill policy overrides.

Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71034/#review216482
-----------------------------------------------------------


Fix it, then Ship it!





src/docker/executor.cpp
Line 401 (original), 404 (patched)
<https://reviews.apache.org/r/71034/#comment303715>

    Should we print the override value here if it is provided?



src/docker/executor.cpp
Lines 948 (patched)
<https://reviews.apache.org/r/71034/#comment303713>

    Unnecessary `move`?



src/docker/executor.cpp
Lines 992 (patched)
<https://reviews.apache.org/r/71034/#comment303714>

    Same as above, unnecessary `std::move()`.



src/exec/exec.cpp
Lines 374 (patched)
<https://reviews.apache.org/r/71034/#comment303716>

    I believe the "standard" way to write this would be
    
    ```
    auto* dockerExecutor = dynamic_cast<docker::DockerExecutor*>(executor);
    if (dockerExecutor) {
      // `executor` was a `DockerExecutor`
    } else {
      // `executor` was some other `Executor`
    }
    ```
    
    This way, classes derived from `DockerExecutor` will also get the correct overload.


- Benno Evers


On July 8, 2019, 6:25 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71034/
> -----------------------------------------------------------
> 
> (Updated July 8, 2019, 6:25 p.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, and Joseph Wu.
> 
> 
> Bugs: MESOS-9853
>     https://issues.apache.org/jira/browse/MESOS-9853
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This adds a new `killTask()` overload to the Docker executor
> and updates the Mesos executor driver to call into that
> overload when the loaded executor is the Docker executor.
> 
> This allows the executor driver to pass the kill policy
> override, when present, into the Docker executor.
> 
> 
> Diffs
> -----
> 
>   src/docker/executor.hpp f21e84c71f646e84404c65fc2ded64bcaff482ef 
>   src/docker/executor.cpp f638e4b65155bcca1be36424b7061ea26a3d6ca3 
>   src/exec/exec.cpp c0fa3b61667da96bc4395bae9956c54446268122 
> 
> 
> Diff: https://reviews.apache.org/r/71034/diff/1/
> 
> 
> Testing
> -------
> 
> Details at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>