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


Given the rather complicated setup of this ( destroy -> Destroyer -...-> 
TasksKiller/s ), I think we really need tests here and also specifically tests 
that cover your addition (freezer-less cgroup destruction).


src/linux/cgroups.cpp (lines 1626 - 1632)
<https://reviews.apache.org/r/36620/#comment151417>

    Why would the freezer component not be available -- can you please mention 
that e.g. a root owned cgroup wont have freezer.state available? Not sure if 
there are other expected reasons for a missing freezer though.



src/linux/cgroups.cpp (lines 1696 - 1697)
<https://reviews.apache.org/r/36620/#comment151406>

    Kill one line plz.



src/linux/cgroups.cpp (lines 1735 - 1737)
<https://reviews.apache.org/r/36620/#comment151409>

    Wouldn't this be a bit better in terms of preventing jaggedness?
    
    ```
          promise.fail(
            "Failed to kill all processes in cgroup: " +
            (processes.isError() ? processes.error() : "processes remain"));
    
    ```



src/linux/cgroups.cpp (line 1778)
<https://reviews.apache.org/r/36620/#comment151407>

    s/freezerCheckError/verify/



src/linux/cgroups.cpp (line 1854)
<https://reviews.apache.org/r/36620/#comment151408>

    s/move/Move/


- Till Toenshoff


On Aug. 24, 2015, 9:33 a.m., Joerg Schad wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> -----------------------------------------------------------
> 
> (Updated Aug. 24, 2015, 9:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
>     https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> WIP Added Non-Freezeer Task Killer.
> 
> 
> Diffs
> -----
> 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
> 
> Diff: https://reviews.apache.org/r/36620/diff/
> 
> 
> Testing
> -------
> 
> sudo make check
> + manual tests
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Reply via email to