mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexander Rojas" <alexan...@mesosphere.io>
Subject Re: Review Request 36620: Added Non-Freezeer Task Killer.
Date Mon, 24 Aug 2015 15:05:28 GMT

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



src/linux/cgroups.cpp (lines 1486 - 1487)
<https://reviews.apache.org/r/36620/#comment151420>

    How about something along the lines of:
    
    > The process uses the cgroups freezer component, which works
    > similar to send a STOP signal to the task but without notifying
    > the task about the signal, to stop execution of tasks before
    > atomically kill the tasks in the group.
    > See https://www.kernel.org/doc/Documentation/cgroups/freezer-subsystem.txt



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

    Not sure about the name of this class, doesn't offer any relevant information: Does it
kill freezer tasks, i.e. tasks of type freezer? Doest it kill frozen tasks, i.e. unresponsive
tasks? 
    
    After reading the documentation from the cgroups freezer on realizes that it uses a freezing
mechanism to stop the tasks before killinh them, but this is not clear neither from the documentation
nor from the name of the class.
    
    How about: `CgroupsFreezerTaskKiller`



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

    weird mix of active and passive voice here. How about:
    
    > … Failure occurs if any process in the cgroup cannot be killed.



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

    This is what I meant when I wrote the comment which appears next, it just makes little
sense to me to discard a future which already failed, since discard purpose is to signal we
are not interested in the result, but if `onFailed` is called, we have the future in a final
state already.



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

    Since there's no control of `processes` being an error (which shouldn't), it makes more
sense for the signature of the method to be `Future<Nothing> kill(set<pid_t>&)`.
And let the called to `kill(processes.get())`. 
    
    It just prevent's someone in the future from making an error and passing a failed `Try`
to `kill()`.



src/linux/cgroups.cpp (lines 1700 - 1701)
<https://reviews.apache.org/r/36620/#comment151426>

    How about:
    
    > Collect futures to the exit statuses of the processes before sending
    > the kill signal ensures we actually caught all exit statuses.



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

    This may be a pattern we use, but I think setting a `Future` to `Failure` to send status
about the content of the `Future` is wrong. Another common pattern is to return a `Try<Nothing>`
or `Option<Error>`, so the failed future indicates an error with the future mechanics
(e.g. the promise when out of scope before setting its value) and the `Try` will pass information
about the operation that it performs.



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

    Do they need to be actual attributes? For example `kill()` could return a `Future<list<Future<Option<int>>>>`,
and `reap()` could take a `list<Future<Option<int>>>` as parameter, then
line 1686 will look:
    
    ```
    chain = kill(processes).then(defer(self(), &Self::reap, lambda::_1));
    ```
    
    and you don't need statuses to be an attribute nor do you need line 1672.



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

    Not sure if `chain` needs to be an attribute. The only reason I see is the discard call
in `finalize()`. But if it needs to be, I feel the name of the attribute and the documentation
are a little bit unfortunate since chain is not a list of reaped processes (their needed of
type process, nor pid_t) but it is actually the exit statuses of those processes. Note that
you cannot even assign which status came from which process.



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

    Again the name, without looking anywhere else, if feels as if we where checking for frozen
tasks and that were an error.


- Alexander Rojas


On Aug. 24, 2015, 11: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, 11: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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message