You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2012/10/29 06:45:23 UTC
Review Request: Re-attempt cgroups killing, when unable to freeze.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7756/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Jie Yu.
Description
-------
We'd like to retry the freeze, kill, thaw, watchEmpty steps when the freezing of the cgroups fails.
This simply retries the above steps when the cgroup does not become empty in the given time interval.
Diffs
-----
src/linux/cgroups.hpp 8147919
src/linux/cgroups.cpp a6056c3
Diff: https://reviews.apache.org/r/7756/diff/
Testing
-------
make check on ubuntu
This version punts on testing for now, but testing suggestions welcome!
Thanks,
Ben Mahler
Re: Review Request: Re-attempt cgroups killing, when unable to freeze.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7756/#review12892
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Hindman
On Oct. 29, 2012, 9:28 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7756/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2012, 9:28 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> Parent Review: https://reviews.apache.org/r/7712/
>
> We'd like to retry the freeze, kill, thaw, watchEmpty steps when the freezing of the cgroups fails.
>
> This simply retries the above steps when the cgroup does not become empty in the given time interval.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 8147919bf7f17e50e047f813db58f99fb9dfffe5
> src/linux/cgroups.cpp a6056c31d504f5570872c0ed123b28400640d5cf
>
> Diff: https://reviews.apache.org/r/7756/diff/
>
>
> Testing
> -------
>
> make check on ubuntu
>
> This version punts on testing for now, but testing suggestions welcome!
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Re-attempt cgroups killing, when unable to freeze.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7756/#review12893
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On Oct. 29, 2012, 9:28 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7756/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2012, 9:28 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> Parent Review: https://reviews.apache.org/r/7712/
>
> We'd like to retry the freeze, kill, thaw, watchEmpty steps when the freezing of the cgroups fails.
>
> This simply retries the above steps when the cgroup does not become empty in the given time interval.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 8147919bf7f17e50e047f813db58f99fb9dfffe5
> src/linux/cgroups.cpp a6056c31d504f5570872c0ed123b28400640d5cf
>
> Diff: https://reviews.apache.org/r/7756/diff/
>
>
> Testing
> -------
>
> make check on ubuntu
>
> This version punts on testing for now, but testing suggestions welcome!
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Re-attempt cgroups killing, when unable to freeze.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7756/
-----------------------------------------------------------
(Updated Oct. 29, 2012, 9:28 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Vinod, jie, benh comments.
Also, eliminated some log lines that are really noisy during retries.
Description
-------
Parent Review: https://reviews.apache.org/r/7712/
We'd like to retry the freeze, kill, thaw, watchEmpty steps when the freezing of the cgroups fails.
This simply retries the above steps when the cgroup does not become empty in the given time interval.
Diffs (updated)
-----
src/linux/cgroups.hpp 8147919bf7f17e50e047f813db58f99fb9dfffe5
src/linux/cgroups.cpp a6056c31d504f5570872c0ed123b28400640d5cf
Diff: https://reviews.apache.org/r/7756/diff/
Testing
-------
make check on ubuntu
This version punts on testing for now, but testing suggestions welcome!
Thanks,
Ben Mahler
Re: Review Request: Re-attempt cgroups killing, when unable to freeze.
Posted by Ben Mahler <be...@gmail.com>.
> On Oct. 29, 2012, 5:59 p.m., Vinod Kone wrote:
> > src/linux/cgroups.cpp, lines 1519-1534
> > <https://reviews.apache.org/r/7756/diff/1/?file=181105#file181105line1519>
> >
> > I understand this code is a copy paste from initialize(), but I don't follow the flow here.
> >
> > Can someone explain?
Discussed offline.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7756/#review12872
-----------------------------------------------------------
On Oct. 29, 2012, 5:45 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7756/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2012, 5:45 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Jie Yu.
>
>
> Description
> -------
>
> Parent Review: https://reviews.apache.org/r/7712/
>
> We'd like to retry the freeze, kill, thaw, watchEmpty steps when the freezing of the cgroups fails.
>
> This simply retries the above steps when the cgroup does not become empty in the given time interval.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 8147919
> src/linux/cgroups.cpp a6056c3
>
> Diff: https://reviews.apache.org/r/7756/diff/
>
>
> Testing
> -------
>
> make check on ubuntu
>
> This version punts on testing for now, but testing suggestions welcome!
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Re-attempt cgroups killing, when unable to freeze.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7756/#review12872
-----------------------------------------------------------
src/linux/cgroups.cpp
<https://reviews.apache.org/r/7756/#comment27675>
hmm..why the revert?
src/linux/cgroups.cpp
<https://reviews.apache.org/r/7756/#comment27678>
I understand this code is a copy paste from initialize(), but I don't follow the flow here.
Can someone explain?
src/linux/cgroups.cpp
<https://reviews.apache.org/r/7756/#comment27677>
s/in/is in/
- Vinod Kone
On Oct. 29, 2012, 5:45 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7756/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2012, 5:45 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Jie Yu.
>
>
> Description
> -------
>
> Parent Review: https://reviews.apache.org/r/7712/
>
> We'd like to retry the freeze, kill, thaw, watchEmpty steps when the freezing of the cgroups fails.
>
> This simply retries the above steps when the cgroup does not become empty in the given time interval.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 8147919
> src/linux/cgroups.cpp a6056c3
>
> Diff: https://reviews.apache.org/r/7756/diff/
>
>
> Testing
> -------
>
> make check on ubuntu
>
> This version punts on testing for now, but testing suggestions welcome!
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Re-attempt cgroups killing, when unable to freeze.
Posted by Ben Mahler <be...@gmail.com>.
> On Oct. 29, 2012, 8:35 p.m., Jie Yu wrote:
> > src/linux/cgroups.cpp, line 1331
> > <https://reviews.apache.org/r/7756/diff/1/?file=181105#file181105line1331>
> >
> > Can we use Options::None() as default here and set "RETRIES" in the client function?
So now, after discussing with benh, I've removed the option and stuck with just unsigned int.
This is because we don't want to expose "blocking" behavior in which the Freezer / EmptyWatcher retries forever.
> On Oct. 29, 2012, 8:35 p.m., Jie Yu wrote:
> > src/linux/cgroups.cpp, line 1374
> > <https://reviews.apache.org/r/7756/diff/1/?file=181105#file181105line1374>
> >
> > Add a comment here. There are three outcomes:
> > 1) return true -> success
> > 2) return false -> retry limit reached
> > 3) return error -> invalid arguments
Added to the future() signature above.
> On Oct. 29, 2012, 8:35 p.m., Jie Yu wrote:
> > src/linux/cgroups.cpp, line 1469
> > <https://reviews.apache.org/r/7756/diff/1/?file=181105#file181105line1469>
> >
> > Here, we do need to check the result of the previous function in the function call chain. So, we should introduce a parameter in these functions and move the comments from freeze() to here.
Good point, moved the comment to kill(). However, I didn't add the boolean result from freeze, since:
1. We still kill / thaw / empty regardless of whether the freeze worked.
2. If the freeze did not work, it gets logged within the Freezer.
I could be convinced otherwise if you had a particular reason for passing in the freeze result?
> On Oct. 29, 2012, 8:35 p.m., Jie Yu wrote:
> > src/linux/cgroups.cpp, line 1517
> > <https://reviews.apache.org/r/7756/diff/1/?file=181105#file181105line1517>
> >
> > Instead of copying the code here, let's refactor it to have a common helper function.
Done. Made a killTasks().
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7756/#review12879
-----------------------------------------------------------
On Oct. 29, 2012, 9:28 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7756/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2012, 9:28 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> Parent Review: https://reviews.apache.org/r/7712/
>
> We'd like to retry the freeze, kill, thaw, watchEmpty steps when the freezing of the cgroups fails.
>
> This simply retries the above steps when the cgroup does not become empty in the given time interval.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 8147919bf7f17e50e047f813db58f99fb9dfffe5
> src/linux/cgroups.cpp a6056c31d504f5570872c0ed123b28400640d5cf
>
> Diff: https://reviews.apache.org/r/7756/diff/
>
>
> Testing
> -------
>
> make check on ubuntu
>
> This version punts on testing for now, but testing suggestions welcome!
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Re-attempt cgroups killing, when unable to freeze.
Posted by Jie Yu <yu...@gmail.com>.
> On Oct. 29, 2012, 8:35 p.m., Jie Yu wrote:
> > src/linux/cgroups.cpp, line 1469
> > <https://reviews.apache.org/r/7756/diff/1/?file=181105#file181105line1469>
> >
> > Here, we do need to check the result of the previous function in the function call chain. So, we should introduce a parameter in these functions and move the comments from freeze() to here.
>
> Ben Mahler wrote:
> Good point, moved the comment to kill(). However, I didn't add the boolean result from freeze, since:
> 1. We still kill / thaw / empty regardless of whether the freeze worked.
> 2. If the freeze did not work, it gets logged within the Freezer.
>
> I could be convinced otherwise if you had a particular reason for passing in the freeze result?
Because people may get confused when reading your comments wondering where the "result" comes from.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7756/#review12879
-----------------------------------------------------------
On Oct. 29, 2012, 9:28 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7756/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2012, 9:28 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> Parent Review: https://reviews.apache.org/r/7712/
>
> We'd like to retry the freeze, kill, thaw, watchEmpty steps when the freezing of the cgroups fails.
>
> This simply retries the above steps when the cgroup does not become empty in the given time interval.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 8147919bf7f17e50e047f813db58f99fb9dfffe5
> src/linux/cgroups.cpp a6056c31d504f5570872c0ed123b28400640d5cf
>
> Diff: https://reviews.apache.org/r/7756/diff/
>
>
> Testing
> -------
>
> make check on ubuntu
>
> This version punts on testing for now, but testing suggestions welcome!
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Re-attempt cgroups killing, when unable to freeze.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7756/#review12879
-----------------------------------------------------------
src/linux/cgroups.cpp
<https://reviews.apache.org/r/7756/#comment27686>
Can we use Options::None() as default here and set "RETRIES" in the client function?
src/linux/cgroups.cpp
<https://reviews.apache.org/r/7756/#comment27684>
Add a comment here. There are three outcomes:
1) return true -> success
2) return false -> retry limit reached
3) return error -> invalid arguments
src/linux/cgroups.cpp
<https://reviews.apache.org/r/7756/#comment27695>
Here, we do need to check the result of the previous function in the function call chain. So, we should introduce a parameter in these functions and move the comments from freeze() to here.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/7756/#comment27687>
Instead of copying the code here, let's refactor it to have a common helper function.
- Jie Yu
On Oct. 29, 2012, 5:45 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7756/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2012, 5:45 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Jie Yu.
>
>
> Description
> -------
>
> Parent Review: https://reviews.apache.org/r/7712/
>
> We'd like to retry the freeze, kill, thaw, watchEmpty steps when the freezing of the cgroups fails.
>
> This simply retries the above steps when the cgroup does not become empty in the given time interval.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 8147919
> src/linux/cgroups.cpp a6056c3
>
> Diff: https://reviews.apache.org/r/7756/diff/
>
>
> Testing
> -------
>
> make check on ubuntu
>
> This version punts on testing for now, but testing suggestions welcome!
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Re-attempt cgroups killing, when unable to freeze.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7756/
-----------------------------------------------------------
(Updated Oct. 29, 2012, 5:45 a.m.)
Review request for mesos, Benjamin Hindman and Jie Yu.
Description (updated)
-------
Parent Review: https://reviews.apache.org/r/7712/
We'd like to retry the freeze, kill, thaw, watchEmpty steps when the freezing of the cgroups fails.
This simply retries the above steps when the cgroup does not become empty in the given time interval.
Diffs
-----
src/linux/cgroups.hpp 8147919
src/linux/cgroups.cpp a6056c3
Diff: https://reviews.apache.org/r/7756/diff/
Testing
-------
make check on ubuntu
This version punts on testing for now, but testing suggestions welcome!
Thanks,
Ben Mahler