You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joerg Schad <jo...@mesosphere.io> on 2015/09/22 21:43:27 UTC
Re: Review Request 37967: Added Non-Freezeer Task Killer. 36620
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37967/
-----------------------------------------------------------
(Updated Sept. 22, 2015, 7:43 p.m.)
Review request for mesos and Till Toenshoff.
Repository: mesos
Description
-------
Added Non-Freezeer Task Killer. 36620
Diffs (updated)
-----
src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835
src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f
Diff: https://reviews.apache.org/r/37967/diff/
Testing
-------
Thanks,
Joerg Schad
Re: Review Request 37967: Added Non-Freezeer Task Killer. 36620
Posted by Jie Yu <yu...@gmail.com>.
> On Sept. 22, 2015, 10:19 p.m., Cong Wang wrote:
> > High level comment: why not just use pid namespace to kill processes since we already have it?
Because not every one wants to use pid namespace (e.g. it'll break thermos).
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37967/#review100070
-----------------------------------------------------------
On Sept. 22, 2015, 7:43 p.m., Joerg Schad wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37967/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2015, 7:43 p.m.)
>
>
> Review request for mesos and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added Non-Freezeer Task Killer. 36620
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835
> src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f
>
> Diff: https://reviews.apache.org/r/37967/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joerg Schad
>
>
Re: Review Request 37967: Added Non-Freezeer Task Killer. 36620
Posted by Cong Wang <cw...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37967/#review100070
-----------------------------------------------------------
High level comment: why not just use pid namespace to kill processes since we already have it?
src/linux/cgroups.hpp (line 758)
<https://reviews.apache.org/r/37967/#comment157180>
Over documented, this is private, so you can just document it in its definition and remove this one.
src/linux/cgroups.cpp (line 981)
<https://reviews.apache.org/r/37967/#comment157179>
s/verified/error/, otherwise reads odd.
src/linux/cgroups.cpp (line 1631)
<https://reviews.apache.org/r/37967/#comment157164>
s/chain/statuses/
- Cong Wang
On Sept. 22, 2015, 7:43 p.m., Joerg Schad wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37967/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2015, 7:43 p.m.)
>
>
> Review request for mesos and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added Non-Freezeer Task Killer. 36620
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835
> src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f
>
> Diff: https://reviews.apache.org/r/37967/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joerg Schad
>
>
Re: Review Request 37967: Added Non-Freezeer Task Killer. 36620
Posted by Joerg Schad <jo...@mesosphere.io>.
> On Sept. 22, 2015, 10:37 p.m., Jie Yu wrote:
> > src/linux/cgroups.hpp, line 666
> > <https://reviews.apache.org/r/37967/diff/2/?file=1081507#file1081507line666>
> >
> > Any reason you want to expose these two classes in the header? If no one is using them right now, please move them to the cpp file (close to their impl.)
I would like to leave them here as my second patch with tests can access the classes in this way.
- Joerg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37967/#review100082
-----------------------------------------------------------
On Sept. 22, 2015, 7:43 p.m., Joerg Schad wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37967/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2015, 7:43 p.m.)
>
>
> Review request for mesos and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added Non-Freezeer Task Killer. 36620
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835
> src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f
>
> Diff: https://reviews.apache.org/r/37967/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joerg Schad
>
>
Re: Review Request 37967: Added Non-Freezeer Task Killer. 36620
Posted by Joerg Schad <jo...@mesosphere.io>.
> On Sept. 22, 2015, 10:37 p.m., Jie Yu wrote:
> > Tests? Also, link the JIRA ticket in the review board.
Updated Review 36620, this was only my internal stash, sorry for adding mesos as Group.
- Joerg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37967/#review100082
-----------------------------------------------------------
On Sept. 22, 2015, 7:43 p.m., Joerg Schad wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37967/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2015, 7:43 p.m.)
>
>
> Review request for mesos and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added Non-Freezeer Task Killer. 36620
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835
> src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f
>
> Diff: https://reviews.apache.org/r/37967/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joerg Schad
>
>
Re: Review Request 37967: Added Non-Freezeer Task Killer. 36620
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37967/#review100082
-----------------------------------------------------------
Tests? Also, link the JIRA ticket in the review board.
src/linux/cgroups.hpp (line 666)
<https://reviews.apache.org/r/37967/#comment157186>
Any reason you want to expose these two classes in the header? If no one is using them right now, please move them to the cpp file (close to their impl.)
src/linux/cgroups.cpp (line 1681)
<https://reviews.apache.org/r/37967/#comment157189>
Indent should be 2 spaces here.
src/linux/cgroups.cpp (line 1689)
<https://reviews.apache.org/r/37967/#comment157190>
No period in the end. Also, can you include the error message here:
```
promise.fail(
"Failed to reap processes of the cgroup: " +
(future.isError() ? future.failure() : "discarded"));
```
- Jie Yu
On Sept. 22, 2015, 7:43 p.m., Joerg Schad wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37967/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2015, 7:43 p.m.)
>
>
> Review request for mesos and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added Non-Freezeer Task Killer. 36620
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835
> src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f
>
> Diff: https://reviews.apache.org/r/37967/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joerg Schad
>
>