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/07/21 17:59:16 UTC
Re: Review Request 36620: WIP Added Non-Freezeer Task Killer.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36620/
-----------------------------------------------------------
(Updated July 21, 2015, 3:59 p.m.)
Review request for mesos and Timothy Chen.
Changes
-------
Style fixes
Repository: mesos
Description
-------
WIP Added Non-Freezeer Task Killer.
Diffs (updated)
-----
src/linux/cgroups.cpp b7d11ac4eb8b78664c8c7bd7a5b0d566b5bf2f0e
Diff: https://reviews.apache.org/r/36620/diff/
Testing
-------
Thanks,
Joerg Schad
Re: Review Request 36620: WIP Added Non-Freezeer Task Killer.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36620/#review92426
-----------------------------------------------------------
Patch looks great!
Reviews applied: [36612, 36620]
All tests passed.
- Mesos ReviewBot
On July 21, 2015, 4:42 p.m., Joerg Schad wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> -----------------------------------------------------------
>
> (Updated July 21, 2015, 4:42 p.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> WIP Added Non-Freezeer Task Killer.
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp b7d11ac4eb8b78664c8c7bd7a5b0d566b5bf2f0e
>
> Diff: https://reviews.apache.org/r/36620/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joerg Schad
>
>
Re: Review Request 36620: WIP Added Non-Freezeer Task Killer.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36620/#review92585
-----------------------------------------------------------
Patch looks great!
Reviews applied: [36612, 36620]
All tests passed.
- Mesos ReviewBot
On July 22, 2015, 10:04 a.m., Joerg Schad wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> -----------------------------------------------------------
>
> (Updated July 22, 2015, 10:04 a.m.)
>
>
> Review request for mesos 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 e062fcbd56315f11882fe0ccb615c490dd719934
>
> Diff: https://reviews.apache.org/r/36620/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Joerg Schad
>
>
Re: Review Request 36620: WIP Added Non-Freezeer Task Killer.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36620/#review92683
-----------------------------------------------------------
src/linux/cgroups.cpp (line 1627)
<https://reviews.apache.org/r/36620/#comment146907>
I think it's good to also describe what the killer does and why it needs to recursively kill.
src/linux/cgroups.cpp (line 1859)
<https://reviews.apache.org/r/36620/#comment146910>
I don't think this log is necessary, at very least VLOG(1)
- Timothy Chen
On July 22, 2015, 10:04 a.m., Joerg Schad wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> -----------------------------------------------------------
>
> (Updated July 22, 2015, 10:04 a.m.)
>
>
> Review request for mesos 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 e062fcbd56315f11882fe0ccb615c490dd719934
>
> Diff: https://reviews.apache.org/r/36620/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Joerg Schad
>
>
Re: Review Request 36620: WIP Added Non-Freezeer Task Killer.
Posted by Joerg Schad <jo...@mesosphere.io>.
> On July 23, 2015, 5:42 p.m., Timothy Chen wrote:
> > src/linux/cgroups.cpp, line 1696
> > <https://reviews.apache.org/r/36620/diff/10/?file=1019693#file1019693line1696>
> >
> > Seems like we can just a lambda instead of a new fail method, then we don't even need to store chain variable right?
We need to store the chain variable for the finalize call, e.g. when the timeout kills the process.
- Joerg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36620/#review92774
-----------------------------------------------------------
On July 24, 2015, 1:07 p.m., Joerg Schad wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> -----------------------------------------------------------
>
> (Updated July 24, 2015, 1:07 p.m.)
>
>
> Review request for mesos, Benjamin Hindman 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 e062fcbd56315f11882fe0ccb615c490dd719934
>
> Diff: https://reviews.apache.org/r/36620/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Joerg Schad
>
>
Re: Review Request 36620: WIP Added Non-Freezeer Task Killer.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36620/#review92774
-----------------------------------------------------------
src/linux/cgroups.cpp (line 1672)
<https://reviews.apache.org/r/36620/#comment147016>
Why we need a new promise here? Why can't we use promise variable you created in the class?
src/linux/cgroups.cpp (line 1678)
<https://reviews.apache.org/r/36620/#comment147017>
"Failed to get cgroup processes: " + processes.error()
src/linux/cgroups.cpp (line 1696)
<https://reviews.apache.org/r/36620/#comment147015>
Seems like we can just a lambda instead of a new fail method, then we don't even need to store chain variable right?
src/linux/cgroups.cpp (line 1713)
<https://reviews.apache.org/r/36620/#comment147014>
I don't think the error message includes the original intention right?
"Failed to cgroups kill: " + kill.error()
- Timothy Chen
On July 23, 2015, 12:21 p.m., Joerg Schad wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> -----------------------------------------------------------
>
> (Updated July 23, 2015, 12:21 p.m.)
>
>
> Review request for mesos 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 e062fcbd56315f11882fe0ccb615c490dd719934
>
> Diff: https://reviews.apache.org/r/36620/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Joerg Schad
>
>
Re: Review Request 36620: WIP Added Non-Freezeer Task Killer.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36620/#review92747
-----------------------------------------------------------
Patch looks great!
Reviews applied: [36612, 36620]
All tests passed.
- Mesos ReviewBot
On July 23, 2015, 12:21 p.m., Joerg Schad wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> -----------------------------------------------------------
>
> (Updated July 23, 2015, 12:21 p.m.)
>
>
> Review request for mesos 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 e062fcbd56315f11882fe0ccb615c490dd719934
>
> Diff: https://reviews.apache.org/r/36620/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Joerg Schad
>
>
Re: Review Request 36620: WIP Added Non-Freezeer Task Killer.
Posted by Joerg Schad <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36620/
-----------------------------------------------------------
(Updated July 23, 2015, 12:21 p.m.)
Review request for mesos and Timothy Chen.
Changes
-------
Improved comments.
Bugs: MESOS-3086
https://issues.apache.org/jira/browse/MESOS-3086
Repository: mesos
Description
-------
WIP Added Non-Freezeer Task Killer.
Diffs (updated)
-----
src/linux/cgroups.cpp e062fcbd56315f11882fe0ccb615c490dd719934
Diff: https://reviews.apache.org/r/36620/diff/
Testing
-------
sudo make check
Thanks,
Joerg Schad
Re: Review Request 36620: WIP Added Non-Freezeer Task Killer.
Posted by Joerg Schad <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36620/
-----------------------------------------------------------
(Updated July 23, 2015, 12:19 p.m.)
Review request for mesos and Timothy Chen.
Changes
-------
Improved comments.
Bugs: MESOS-3086
https://issues.apache.org/jira/browse/MESOS-3086
Repository: mesos
Description
-------
WIP Added Non-Freezeer Task Killer.
Diffs (updated)
-----
src/linux/cgroups.cpp e062fcbd56315f11882fe0ccb615c490dd719934
Diff: https://reviews.apache.org/r/36620/diff/
Testing
-------
sudo make check
Thanks,
Joerg Schad
Re: Review Request 36620: WIP Added Non-Freezeer Task Killer.
Posted by Joerg Schad <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36620/
-----------------------------------------------------------
(Updated July 22, 2015, 10:04 a.m.)
Review request for mesos and Timothy Chen.
Bugs: MESOS-3086
https://issues.apache.org/jira/browse/MESOS-3086
Repository: mesos
Description
-------
WIP Added Non-Freezeer Task Killer.
Diffs (updated)
-----
src/linux/cgroups.cpp e062fcbd56315f11882fe0ccb615c490dd719934
Diff: https://reviews.apache.org/r/36620/diff/
Testing
-------
sudo make check
Thanks,
Joerg Schad
Re: Review Request 36620: WIP Added Non-Freezeer Task Killer.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36620/#review92549
-----------------------------------------------------------
Bad patch!
Reviews applied: [36612]
Failed command: ./support/apply-review.sh -n -r 36612
Error:
2015-07-22 05:28:09 URL:https://reviews.apache.org/r/36612/diff/raw/ [12620/12620] -> "36612.patch" [1]
error: patch failed: src/linux/cgroups.hpp:92
error: src/linux/cgroups.hpp: patch does not apply
error: patch failed: src/linux/cgroups.cpp:110
error: src/linux/cgroups.cpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On July 22, 2015, 4:37 a.m., Joerg Schad wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> -----------------------------------------------------------
>
> (Updated July 22, 2015, 4:37 a.m.)
>
>
> Review request for mesos 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 b7d11ac4eb8b78664c8c7bd7a5b0d566b5bf2f0e
>
> Diff: https://reviews.apache.org/r/36620/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Joerg Schad
>
>
Re: Review Request 36620: WIP Added Non-Freezeer Task Killer.
Posted by Joerg Schad <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36620/
-----------------------------------------------------------
(Updated July 22, 2015, 4:37 a.m.)
Review request for mesos and Timothy Chen.
Bugs: MESOS-3086
https://issues.apache.org/jira/browse/MESOS-3086
Repository: mesos
Description
-------
WIP Added Non-Freezeer Task Killer.
Diffs (updated)
-----
src/linux/cgroups.cpp b7d11ac4eb8b78664c8c7bd7a5b0d566b5bf2f0e
Diff: https://reviews.apache.org/r/36620/diff/
Testing
-------
sudo make check
Thanks,
Joerg Schad
Re: Review Request 36620: WIP Added Non-Freezeer Task Killer.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36620/#review92457
-----------------------------------------------------------
src/linux/cgroups.cpp (line 1504)
<https://reviews.apache.org/r/36620/#comment146628>
This is actually the Freezer right?
And we should make this VLOG(1)
src/linux/cgroups.cpp (line 1627)
<https://reviews.apache.org/r/36620/#comment146629>
To me this doesn't killer doesn't "atomically" kill all of them, as that implies either all or none.
I think the TaskKiller actually best effort try to kill by signal and reintrospect all tasks in a cgroup within the timeout
src/linux/cgroups.cpp (line 1650)
<https://reviews.apache.org/r/36620/#comment146630>
100 ms seems way too fast?
And why not use the default timeout that's already there?
src/linux/cgroups.cpp (line 1668)
<https://reviews.apache.org/r/36620/#comment146632>
This should go to line 1674?
src/linux/cgroups.cpp (line 1676)
<https://reviews.apache.org/r/36620/#comment146631>
Move to previous line.
src/linux/cgroups.cpp (line 1684)
<https://reviews.apache.org/r/36620/#comment146633>
This fits on to previous line?
- Timothy Chen
On July 21, 2015, 5:18 p.m., Joerg Schad wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> -----------------------------------------------------------
>
> (Updated July 21, 2015, 5:18 p.m.)
>
>
> Review request for mesos 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 b7d11ac4eb8b78664c8c7bd7a5b0d566b5bf2f0e
>
> Diff: https://reviews.apache.org/r/36620/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Joerg Schad
>
>
Re: Review Request 36620: WIP Added Non-Freezeer Task Killer.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36620/#review92459
-----------------------------------------------------------
src/linux/cgroups.cpp (line 1671)
<https://reviews.apache.org/r/36620/#comment146638>
It's very odd to see the failure message set here but then a lot more attempts to try to set it in the finished callback.
How about having a failed method that sets the failure and another finished callback that is what you have?
src/linux/cgroups.cpp (line 1683)
<https://reviews.apache.org/r/36620/#comment146637>
Very wierd identation?
src/linux/cgroups.cpp (line 1686)
<https://reviews.apache.org/r/36620/#comment146634>
//check -> // Check
src/linux/cgroups.cpp (line 1688)
<https://reviews.apache.org/r/36620/#comment146636>
one more space to the right
src/linux/cgroups.cpp (line 1708)
<https://reviews.apache.org/r/36620/#comment146635>
extra space between "the" and "pids"
- Timothy Chen
On July 21, 2015, 5:18 p.m., Joerg Schad wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> -----------------------------------------------------------
>
> (Updated July 21, 2015, 5:18 p.m.)
>
>
> Review request for mesos 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 b7d11ac4eb8b78664c8c7bd7a5b0d566b5bf2f0e
>
> Diff: https://reviews.apache.org/r/36620/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Joerg Schad
>
>
Re: Review Request 36620: WIP Added Non-Freezeer Task Killer.
Posted by Joerg Schad <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36620/
-----------------------------------------------------------
(Updated July 21, 2015, 5:18 p.m.)
Review request for mesos 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 b7d11ac4eb8b78664c8c7bd7a5b0d566b5bf2f0e
Diff: https://reviews.apache.org/r/36620/diff/
Testing (updated)
-------
sudo make check
Thanks,
Joerg Schad
Re: Review Request 36620: WIP Added Non-Freezeer Task Killer.
Posted by Joerg Schad <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36620/
-----------------------------------------------------------
(Updated July 21, 2015, 5:17 p.m.)
Review request for mesos 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 b7d11ac4eb8b78664c8c7bd7a5b0d566b5bf2f0e
Diff: https://reviews.apache.org/r/36620/diff/
Testing
-------
Thanks,
Joerg Schad
Re: Review Request 36620: WIP Added Non-Freezeer Task Killer.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36620/#review92432
-----------------------------------------------------------
Could you please attach a ticket or explain in the description about the motivation?
- Jie Yu
On July 21, 2015, 4:42 p.m., Joerg Schad wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> -----------------------------------------------------------
>
> (Updated July 21, 2015, 4:42 p.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> WIP Added Non-Freezeer Task Killer.
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp b7d11ac4eb8b78664c8c7bd7a5b0d566b5bf2f0e
>
> Diff: https://reviews.apache.org/r/36620/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joerg Schad
>
>
Re: Review Request 36620: WIP Added Non-Freezeer Task Killer.
Posted by Joerg Schad <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36620/
-----------------------------------------------------------
(Updated July 21, 2015, 4:42 p.m.)
Review request for mesos and Timothy Chen.
Repository: mesos
Description
-------
WIP Added Non-Freezeer Task Killer.
Diffs (updated)
-----
src/linux/cgroups.cpp b7d11ac4eb8b78664c8c7bd7a5b0d566b5bf2f0e
Diff: https://reviews.apache.org/r/36620/diff/
Testing
-------
Thanks,
Joerg Schad