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 2014/01/24 08:05:38 UTC
Review Request 17304: Added a child Reaper utility in libprocess.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17304/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
Bugs: MESOS-943
https://issues.apache.org/jira/browse/MESOS-943
Repository: mesos-git
Description
-------
This is a copy of the Reaper from Mesos, with one key difference: This now only reaps pids that were monitored explicitly.
The test needed updating to reflect the fact that we can now retrieve the exit status for an already exited process.
This also opens up the opportunity to eliminate the 1 second delay in the Reaper (using threads or SIGCHLD), but this is a copy of the Mesos Reaper for now.
Diffs
-----
3rdparty/libprocess/Makefile.am 40f01a7b3803696ccca440c8326e1d6d7c377459
3rdparty/libprocess/include/process/reaper.hpp PRE-CREATION
3rdparty/libprocess/src/reaper.cpp PRE-CREATION
3rdparty/libprocess/src/tests/reaper_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/17304/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request 17304: Added a child Reaper utility in libprocess.
Posted by Ben Mahler <be...@gmail.com>.
> On Jan. 24, 2014, 8:15 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/src/reaper.cpp, lines 70-98
> > <https://reviews.apache.org/r/17304/diff/1/?file=447748#file447748line70>
> >
> > Any reason why these two loops cannot be merged now?
Definitely! Thanks for pointing this out :)
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17304/#review32744
-----------------------------------------------------------
On Jan. 24, 2014, 7:05 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17304/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2014, 7:05 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Bugs: MESOS-943
> https://issues.apache.org/jira/browse/MESOS-943
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is a copy of the Reaper from Mesos, with one key difference: This now only reaps pids that were monitored explicitly.
>
> The test needed updating to reflect the fact that we can now retrieve the exit status for an already exited process.
>
> This also opens up the opportunity to eliminate the 1 second delay in the Reaper (using threads or SIGCHLD), but this is a copy of the Mesos Reaper for now.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 40f01a7b3803696ccca440c8326e1d6d7c377459
> 3rdparty/libprocess/include/process/reaper.hpp PRE-CREATION
> 3rdparty/libprocess/src/reaper.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/reaper_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17304/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 17304: Added a child Reaper utility in libprocess.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17304/#review32744
-----------------------------------------------------------
3rdparty/libprocess/src/reaper.cpp
<https://reviews.apache.org/r/17304/#comment61706>
Any reason why these two loops cannot be merged now?
- Vinod Kone
On Jan. 24, 2014, 7:05 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17304/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2014, 7:05 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Bugs: MESOS-943
> https://issues.apache.org/jira/browse/MESOS-943
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is a copy of the Reaper from Mesos, with one key difference: This now only reaps pids that were monitored explicitly.
>
> The test needed updating to reflect the fact that we can now retrieve the exit status for an already exited process.
>
> This also opens up the opportunity to eliminate the 1 second delay in the Reaper (using threads or SIGCHLD), but this is a copy of the Mesos Reaper for now.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 40f01a7b3803696ccca440c8326e1d6d7c377459
> 3rdparty/libprocess/include/process/reaper.hpp PRE-CREATION
> 3rdparty/libprocess/src/reaper.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/reaper_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17304/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 17304: Added a child Reaper utility in libprocess.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17304/#review32751
-----------------------------------------------------------
3rdparty/libprocess/src/reaper.cpp
<https://reviews.apache.org/r/17304/#comment61713>
No, should've just been a comment.
- Ian Downes
On Jan. 24, 2014, 7:05 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17304/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2014, 7:05 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Bugs: MESOS-943
> https://issues.apache.org/jira/browse/MESOS-943
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is a copy of the Reaper from Mesos, with one key difference: This now only reaps pids that were monitored explicitly.
>
> The test needed updating to reflect the fact that we can now retrieve the exit status for an already exited process.
>
> This also opens up the opportunity to eliminate the 1 second delay in the Reaper (using threads or SIGCHLD), but this is a copy of the Mesos Reaper for now.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 40f01a7b3803696ccca440c8326e1d6d7c377459
> 3rdparty/libprocess/include/process/reaper.hpp PRE-CREATION
> 3rdparty/libprocess/src/reaper.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/reaper_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17304/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 17304: Added a child Reaper utility in libprocess.
Posted by Ben Mahler <be...@gmail.com>.
> On Jan. 29, 2014, 7:03 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/src/reap.cpp, line 79
> > <https://reviews.apache.org/r/17304/diff/3/?file=453443#file453443line79>
> >
> > We had discussed this back when Yan worked on this code ... but I don't remember what we determined and there isn't a comment. :( Does this mean we might give false positives? What's worse, a false positive or never telling the user about a terminated process? Note that above you made an error be a Failure.
Yes good catch, I've updated this to be considered a failed future.
> On Jan. 29, 2014, 7:03 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/src/tests/reap_tests.cpp, line 94
> > <https://reviews.apache.org/r/17304/diff/3/?file=453444#file453444line94>
> >
> > Maybe a comment as to why you're not ASSERT_SOME(status) and/or checking that status.get().get() is equal to 0?
Added the asserts.
> On Jan. 29, 2014, 7:03 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/src/tests/reap_tests.cpp, line 130
> > <https://reviews.apache.org/r/17304/diff/3/?file=453444#file453444line130>
> >
> > I know this isn't yours but I'd prefer to not make this a convention, I don't see any problem doing status.get().get() in the next two lines (in the next test too).
Unfortunately it doesn't compile due to the way the macros were implemented:
../../../3rdparty/libprocess/src/tests/reap_tests.cpp:88: error: lvalue required as unary ‘&’ operand
../../../3rdparty/libprocess/src/tests/reap_tests.cpp:89: error: lvalue required as unary ‘&’ operand
> On Jan. 29, 2014, 7:03 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/src/tests/reap_tests.cpp, lines 62-64
> > <https://reviews.apache.org/r/17304/diff/3/?file=453444#file453444line62>
> >
> > Why is waiting 5 milliseconds sufficient? Seems like if you really wanted to check this you'd EXPECT_DISPATCH on RepearProcess::wait and expect the status to still be pending after Clock::settle. Unless I'm missing something the 5 milliseconds seems like it's misleading the reader.
It's technically not sufficient, I added it to compensate for the fact that the previous code was expecting on dispatches, but I don't think it is necessary for this test so I've removed it.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17304/#review33104
-----------------------------------------------------------
On Jan. 29, 2014, 3:39 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17304/
> -----------------------------------------------------------
>
> (Updated Jan. 29, 2014, 3:39 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Bugs: MESOS-943
> https://issues.apache.org/jira/browse/MESOS-943
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is a copy of the Reaper from Mesos, with one key difference: This now only reaps pids that were monitored explicitly.
>
> The test needed updating to reflect the fact that we can now retrieve the exit status for an already exited process.
>
> This also opens up the opportunity to eliminate the 1 second delay in the Reaper (using threads or SIGCHLD), but this is a copy of the Mesos Reaper for now.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 40f01a7b3803696ccca440c8326e1d6d7c377459
> 3rdparty/libprocess/include/process/reap.hpp PRE-CREATION
> 3rdparty/libprocess/src/reap.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/reap_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17304/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 17304: Added a child Reaper utility in libprocess.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17304/#review33104
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/include/process/reap.hpp
<https://reviews.apache.org/r/17304/#comment62368>
A suggestion:
Returns the exit status of the specified process if and only if the process is a direct child and it has not already been reaped. Otherwise, returns None once the process has been reaped elsewhere (or does not exist which is indistinguishable from being reaped elsewhere).
3rdparty/libprocess/include/process/reap.hpp
<https://reviews.apache.org/r/17304/#comment62369>
// namespace process {
3rdparty/libprocess/src/reap.cpp
<https://reviews.apache.org/r/17304/#comment62370>
The comment is great and could actually be the comment for process::reap!
3rdparty/libprocess/src/reap.cpp
<https://reviews.apache.org/r/17304/#comment62371>
We had discussed this back when Yan worked on this code ... but I don't remember what we determined and there isn't a comment. :( Does this mean we might give false positives? What's worse, a false positive or never telling the user about a terminated process? Note that above you made an error be a Failure.
3rdparty/libprocess/src/tests/reap_tests.cpp
<https://reviews.apache.org/r/17304/#comment62372>
Is this comment still true after MESOS-952 is fixed?
3rdparty/libprocess/src/tests/reap_tests.cpp
<https://reviews.apache.org/r/17304/#comment62374>
Why is waiting 5 milliseconds sufficient? Seems like if you really wanted to check this you'd EXPECT_DISPATCH on RepearProcess::wait and expect the status to still be pending after Clock::settle. Unless I'm missing something the 5 milliseconds seems like it's misleading the reader.
3rdparty/libprocess/src/tests/reap_tests.cpp
<https://reviews.apache.org/r/17304/#comment62375>
s/executor/grandchild/
3rdparty/libprocess/src/tests/reap_tests.cpp
<https://reviews.apache.org/r/17304/#comment62376>
Maybe a comment as to why you're not ASSERT_SOME(status) and/or checking that status.get().get() is equal to 0?
3rdparty/libprocess/src/tests/reap_tests.cpp
<https://reviews.apache.org/r/17304/#comment62373>
I know this isn't yours but I'd prefer to not make this a convention, I don't see any problem doing status.get().get() in the next two lines (in the next test too).
- Benjamin Hindman
On Jan. 29, 2014, 3:39 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17304/
> -----------------------------------------------------------
>
> (Updated Jan. 29, 2014, 3:39 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Bugs: MESOS-943
> https://issues.apache.org/jira/browse/MESOS-943
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is a copy of the Reaper from Mesos, with one key difference: This now only reaps pids that were monitored explicitly.
>
> The test needed updating to reflect the fact that we can now retrieve the exit status for an already exited process.
>
> This also opens up the opportunity to eliminate the 1 second delay in the Reaper (using threads or SIGCHLD), but this is a copy of the Mesos Reaper for now.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 40f01a7b3803696ccca440c8326e1d6d7c377459
> 3rdparty/libprocess/include/process/reap.hpp PRE-CREATION
> 3rdparty/libprocess/src/reap.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/reap_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17304/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 17304: Added a child Reaper utility in libprocess.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17304/
-----------------------------------------------------------
(Updated Jan. 29, 2014, 10:38 p.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, Jie Yu, and Vinod Kone.
Changes
-------
Re-added 'Depends On' field.
Bugs: MESOS-943
https://issues.apache.org/jira/browse/MESOS-943
Repository: mesos-git
Description
-------
This is a copy of the Reaper from Mesos, with one key difference: This now only reaps pids that were monitored explicitly.
The test needed updating to reflect the fact that we can now retrieve the exit status for an already exited process.
This also opens up the opportunity to eliminate the 1 second delay in the Reaper (using threads or SIGCHLD), but this is a copy of the Mesos Reaper for now.
Diffs
-----
3rdparty/libprocess/Makefile.am 40f01a7b3803696ccca440c8326e1d6d7c377459
3rdparty/libprocess/include/process/reap.hpp PRE-CREATION
3rdparty/libprocess/src/reap.cpp PRE-CREATION
3rdparty/libprocess/src/tests/reap_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/17304/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request 17304: Added a child Reaper utility in libprocess.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17304/
-----------------------------------------------------------
(Updated Jan. 29, 2014, 10:29 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Updated notify to take a Result to handle failures.
Other minor cleanups from benh review.
Bugs: MESOS-943
https://issues.apache.org/jira/browse/MESOS-943
Repository: mesos-git
Description
-------
This is a copy of the Reaper from Mesos, with one key difference: This now only reaps pids that were monitored explicitly.
The test needed updating to reflect the fact that we can now retrieve the exit status for an already exited process.
This also opens up the opportunity to eliminate the 1 second delay in the Reaper (using threads or SIGCHLD), but this is a copy of the Mesos Reaper for now.
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am 40f01a7b3803696ccca440c8326e1d6d7c377459
3rdparty/libprocess/include/process/reap.hpp PRE-CREATION
3rdparty/libprocess/src/reap.cpp PRE-CREATION
3rdparty/libprocess/src/tests/reap_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/17304/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request 17304: Added a child Reaper utility in libprocess.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17304/
-----------------------------------------------------------
(Updated Jan. 29, 2014, 3:39 a.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
Changes
-------
Cleaned up includes.
Bugs: MESOS-943
https://issues.apache.org/jira/browse/MESOS-943
Repository: mesos-git
Description
-------
This is a copy of the Reaper from Mesos, with one key difference: This now only reaps pids that were monitored explicitly.
The test needed updating to reflect the fact that we can now retrieve the exit status for an already exited process.
This also opens up the opportunity to eliminate the 1 second delay in the Reaper (using threads or SIGCHLD), but this is a copy of the Mesos Reaper for now.
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am 40f01a7b3803696ccca440c8326e1d6d7c377459
3rdparty/libprocess/include/process/reap.hpp PRE-CREATION
3rdparty/libprocess/src/reap.cpp PRE-CREATION
3rdparty/libprocess/src/tests/reap_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/17304/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request 17304: Added a child Reaper utility in libprocess.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17304/
-----------------------------------------------------------
(Updated Jan. 27, 2014, 11:25 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
This now is exposed as a library utility, rather than a Reaper object:
Future<Option<int> > process::reap(pid_t);
Which uses a lazily initialized global reaper. This is more efficient, however, the reap related tests take increasingly long when run in repetition due to the Reaper's wait() loop getting scheduled further and further into the future (more calls to Clock::advance / Clock::settle become necessary as the tests run in repetition).
Bugs: MESOS-943
https://issues.apache.org/jira/browse/MESOS-943
Repository: mesos-git
Description
-------
This is a copy of the Reaper from Mesos, with one key difference: This now only reaps pids that were monitored explicitly.
The test needed updating to reflect the fact that we can now retrieve the exit status for an already exited process.
This also opens up the opportunity to eliminate the 1 second delay in the Reaper (using threads or SIGCHLD), but this is a copy of the Mesos Reaper for now.
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am 40f01a7b3803696ccca440c8326e1d6d7c377459
3rdparty/libprocess/include/process/reap.hpp PRE-CREATION
3rdparty/libprocess/src/reap.cpp PRE-CREATION
3rdparty/libprocess/src/tests/reap_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/17304/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request 17304: Added a child Reaper utility in libprocess.
Posted by Ben Mahler <be...@gmail.com>.
> On Jan. 24, 2014, 5:46 p.m., Ian Downes wrote:
> > 3rdparty/libprocess/src/reaper.cpp, line 101
> > <https://reviews.apache.org/r/17304/diff/1/?file=447748#file447748line101>
> >
> > Out of curiosity, why was 1 second originally chosen?
It was likely an arbitrary decision based on a compromise between having too many calls to waitpid vs. faster reap notifications. (You labelled this as an 'issue', do you think it merits a comment?)
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17304/#review32723
-----------------------------------------------------------
On Jan. 24, 2014, 7:05 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17304/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2014, 7:05 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Bugs: MESOS-943
> https://issues.apache.org/jira/browse/MESOS-943
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is a copy of the Reaper from Mesos, with one key difference: This now only reaps pids that were monitored explicitly.
>
> The test needed updating to reflect the fact that we can now retrieve the exit status for an already exited process.
>
> This also opens up the opportunity to eliminate the 1 second delay in the Reaper (using threads or SIGCHLD), but this is a copy of the Mesos Reaper for now.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 40f01a7b3803696ccca440c8326e1d6d7c377459
> 3rdparty/libprocess/include/process/reaper.hpp PRE-CREATION
> 3rdparty/libprocess/src/reaper.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/reaper_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17304/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 17304: Added a child Reaper utility in libprocess.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17304/#review32723
-----------------------------------------------------------
3rdparty/libprocess/include/process/reaper.hpp
<https://reviews.apache.org/r/17304/#comment61672>
Is this comment redundant now since we only wait on explicitly registered pids?
3rdparty/libprocess/src/reaper.cpp
<https://reviews.apache.org/r/17304/#comment61673>
Out of curiosity, why was 1 second originally chosen?
3rdparty/libprocess/src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/17304/#comment61674>
s/grand child/grandchild/
3rdparty/libprocess/src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/17304/#comment61675>
s/grand child/grandchild/
3rdparty/libprocess/src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/17304/#comment61676>
s/grand child/grandchild/
3rdparty/libprocess/src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/17304/#comment61677>
s/executor/child/?
3rdparty/libprocess/src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/17304/#comment61678>
s/grand child/child/
- Ian Downes
On Jan. 24, 2014, 7:05 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17304/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2014, 7:05 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Bugs: MESOS-943
> https://issues.apache.org/jira/browse/MESOS-943
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is a copy of the Reaper from Mesos, with one key difference: This now only reaps pids that were monitored explicitly.
>
> The test needed updating to reflect the fact that we can now retrieve the exit status for an already exited process.
>
> This also opens up the opportunity to eliminate the 1 second delay in the Reaper (using threads or SIGCHLD), but this is a copy of the Mesos Reaper for now.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 40f01a7b3803696ccca440c8326e1d6d7c377459
> 3rdparty/libprocess/include/process/reaper.hpp PRE-CREATION
> 3rdparty/libprocess/src/reaper.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/reaper_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17304/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 17304: Added a child Reaper utility in libprocess.
Posted by Ben Mahler <be...@gmail.com>.
> On Jan. 24, 2014, 7:40 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/src/reaper.cpp, lines 31-34
> > <https://reviews.apache.org/r/17304/diff/1/?file=447748#file447748line31>
> >
> > Not your bug, but any reason we can't reuse an existing promise if it's already in the map?
It seems that two callers reaping the same pid should not be a supported use-case, given the semantics of process reaping.
Now that I've changed the behavior, duplicate calls will result in duplicate calls to waitpid(pid), which is not the right thing to do. We could re-use the promise, however, this opens up a race:
-> reap(pid);
-> waitpid(pid)
-> promises[pid] satisfied and deleted
-> reap(pid) duplicate call arrives in the queue, but does not receive the exit status, as we've already reaped the pid
Given this, I think we should not support two calls to reap against the same pid, so I've opted to return a Failure when this is detected instead. Sounds good?
> On Jan. 24, 2014, 7:40 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/src/reaper.cpp, lines 36-37
> > <https://reviews.apache.org/r/17304/diff/1/?file=447748#file447748line36>
> >
> > Since we've moved this into a library let's kill this LOG or make it VLOG. It should be expected that a user of this would want to print something like that out if they got back a None. Same for the logging statements below.
Good catch!
> On Jan. 24, 2014, 7:40 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/reaper.hpp, line 40
> > <https://reviews.apache.org/r/17304/diff/1/?file=447747#file447747line40>
> >
> > As long as we're making this cleanup, how do you feel about s/monitor/reap/ and s/reap/wait/ below?
Thanks for the poke! My plan was to expose this as:
Future<Option<int> > process::reap(pid_t);
And to use a static reaper object under the covers. Will update with this shortly.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17304/#review32704
-----------------------------------------------------------
On Jan. 24, 2014, 7:05 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17304/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2014, 7:05 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Bugs: MESOS-943
> https://issues.apache.org/jira/browse/MESOS-943
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is a copy of the Reaper from Mesos, with one key difference: This now only reaps pids that were monitored explicitly.
>
> The test needed updating to reflect the fact that we can now retrieve the exit status for an already exited process.
>
> This also opens up the opportunity to eliminate the 1 second delay in the Reaper (using threads or SIGCHLD), but this is a copy of the Mesos Reaper for now.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 40f01a7b3803696ccca440c8326e1d6d7c377459
> 3rdparty/libprocess/include/process/reaper.hpp PRE-CREATION
> 3rdparty/libprocess/src/reaper.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/reaper_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17304/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 17304: Added a child Reaper utility in libprocess.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17304/#review32704
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/include/process/reaper.hpp
<https://reviews.apache.org/r/17304/#comment61644>
As long as we're making this cleanup, how do you feel about s/monitor/reap/ and s/reap/wait/ below?
3rdparty/libprocess/src/reaper.cpp
<https://reviews.apache.org/r/17304/#comment61643>
Not your bug, but any reason we can't reuse an existing promise if it's already in the map?
3rdparty/libprocess/src/reaper.cpp
<https://reviews.apache.org/r/17304/#comment61642>
Since we've moved this into a library let's kill this LOG or make it VLOG. It should be expected that a user of this would want to print something like that out if they got back a None. Same for the logging statements below.
- Benjamin Hindman
On Jan. 24, 2014, 7:05 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17304/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2014, 7:05 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Bugs: MESOS-943
> https://issues.apache.org/jira/browse/MESOS-943
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is a copy of the Reaper from Mesos, with one key difference: This now only reaps pids that were monitored explicitly.
>
> The test needed updating to reflect the fact that we can now retrieve the exit status for an already exited process.
>
> This also opens up the opportunity to eliminate the 1 second delay in the Reaper (using threads or SIGCHLD), but this is a copy of the Mesos Reaper for now.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 40f01a7b3803696ccca440c8326e1d6d7c377459
> 3rdparty/libprocess/include/process/reaper.hpp PRE-CREATION
> 3rdparty/libprocess/src/reaper.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/reaper_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17304/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 17304: Added a child Reaper utility in libprocess.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17304/#review32734
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On Jan. 24, 2014, 7:05 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17304/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2014, 7:05 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Bugs: MESOS-943
> https://issues.apache.org/jira/browse/MESOS-943
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is a copy of the Reaper from Mesos, with one key difference: This now only reaps pids that were monitored explicitly.
>
> The test needed updating to reflect the fact that we can now retrieve the exit status for an already exited process.
>
> This also opens up the opportunity to eliminate the 1 second delay in the Reaper (using threads or SIGCHLD), but this is a copy of the Mesos Reaper for now.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 40f01a7b3803696ccca440c8326e1d6d7c377459
> 3rdparty/libprocess/include/process/reaper.hpp PRE-CREATION
> 3rdparty/libprocess/src/reaper.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/reaper_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17304/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 17304: Added a child Reaper utility in libprocess.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17304/#review32724
-----------------------------------------------------------
Ship it!
Ship It!
- Ian Downes
On Jan. 24, 2014, 7:05 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17304/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2014, 7:05 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Bugs: MESOS-943
> https://issues.apache.org/jira/browse/MESOS-943
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is a copy of the Reaper from Mesos, with one key difference: This now only reaps pids that were monitored explicitly.
>
> The test needed updating to reflect the fact that we can now retrieve the exit status for an already exited process.
>
> This also opens up the opportunity to eliminate the 1 second delay in the Reaper (using threads or SIGCHLD), but this is a copy of the Mesos Reaper for now.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 40f01a7b3803696ccca440c8326e1d6d7c377459
> 3rdparty/libprocess/include/process/reaper.hpp PRE-CREATION
> 3rdparty/libprocess/src/reaper.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/reaper_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17304/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>