You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jiang Yan Xu <ya...@jxu.me> on 2013/04/24 02:51:18 UTC
Review Request: Change reaper to use an Option as process exit status.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10747/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
Summary (updated)
-----------------
Change reaper to use an Option as process exit status.
Description (updated)
-------
See summary.
Diffs (updated)
-----
src/slave/cgroups_isolator.hpp f8fabc4e1c3c303b35a76db96b4b2479bd7c8ff8
src/slave/cgroups_isolator.cpp 8b79da50d8fb0c2c8716dd7d2c734b65c32f60b4
src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
src/slave/process_isolator.cpp 6e2af87d291d7c3448393c1ffa816f7020e2dff6
src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
src/slave/slave.hpp f9d9c2b32983700fb09b266a0abf2a396174ae05
src/slave/slave.cpp a0d3864d3e02c7889646f9ff004dad82f67d263b
src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
Diff: https://reviews.apache.org/r/10747/diff/
Testing (updated)
-------
make check.
Thanks,
Jiang Yan Xu
Re: Review Request: Change reaper to use an Option as process exit status.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On April 24, 2013, 9:27 p.m., Benjamin Hindman wrote:
> > src/tests/reaper_tests.cpp, line 225
> > <https://reviews.apache.org/r/10747/diff/1/?file=283987#file283987line225>
> >
> > Seems like we need an ASSERT_NONE in stout/gtest.hpp!
Added in another commit.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10747/#review19666
-----------------------------------------------------------
On April 26, 2013, 7:57 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10747/
> -----------------------------------------------------------
>
> (Updated April 26, 2013, 7:57 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolator.hpp f8fabc4e1c3c303b35a76db96b4b2479bd7c8ff8
> src/slave/cgroups_isolator.cpp 8b79da50d8fb0c2c8716dd7d2c734b65c32f60b4
> src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
> src/slave/process_isolator.cpp 6e2af87d291d7c3448393c1ffa816f7020e2dff6
> src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
> src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
> src/slave/slave.hpp f9d9c2b32983700fb09b266a0abf2a396174ae05
> src/slave/slave.cpp a0d3864d3e02c7889646f9ff004dad82f67d263b
> src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
>
> Diff: https://reviews.apache.org/r/10747/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Change reaper to use an Option as process exit status.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10747/#review19666
-----------------------------------------------------------
src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/10747/#comment40606>
Use ASSERT_SOME.
src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/10747/#comment40607>
Then you'll have to use a temp, I suggest 'status_' (pronounced "status prime").
src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/10747/#comment40608>
Seems like we need an ASSERT_NONE in stout/gtest.hpp!
- Benjamin Hindman
On April 24, 2013, 12:51 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10747/
> -----------------------------------------------------------
>
> (Updated April 24, 2013, 12:51 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolator.hpp f8fabc4e1c3c303b35a76db96b4b2479bd7c8ff8
> src/slave/cgroups_isolator.cpp 8b79da50d8fb0c2c8716dd7d2c734b65c32f60b4
> src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
> src/slave/process_isolator.cpp 6e2af87d291d7c3448393c1ffa816f7020e2dff6
> src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
> src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
> src/slave/slave.hpp f9d9c2b32983700fb09b266a0abf2a396174ae05
> src/slave/slave.cpp a0d3864d3e02c7889646f9ff004dad82f67d263b
> src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
>
> Diff: https://reviews.apache.org/r/10747/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Change reaper to use an Option as process exit status.
Posted by Ben Mahler <be...@gmail.com>.
> On April 26, 2013, 9:56 p.m., Ben Mahler wrote:
> > src/slave/cgroups_isolator.cpp, line 822
> > <https://reviews.apache.org/r/10747/diff/2/?file=285041#file285041line822>
> >
> > is stringify needed?
>
> Jiang Yan Xu wrote:
> Yes because the ternary operator requires the elements on each side of ':' to be of the same type.
Sort of, it's actually quite complicated in C++: http://stackoverflow.com/a/8535257
But I'm assuming you tried it here ;)
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10747/#review19806
-----------------------------------------------------------
On April 29, 2013, 7:34 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10747/
> -----------------------------------------------------------
>
> (Updated April 29, 2013, 7:34 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolator.hpp f8fabc4e1c3c303b35a76db96b4b2479bd7c8ff8
> src/slave/cgroups_isolator.cpp 8b79da50d8fb0c2c8716dd7d2c734b65c32f60b4
> src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
> src/slave/process_isolator.cpp 6e2af87d291d7c3448393c1ffa816f7020e2dff6
> src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
> src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
> src/slave/slave.hpp f9d9c2b32983700fb09b266a0abf2a396174ae05
> src/slave/slave.cpp 86a15fce93e6aa3f7a8f9700ef2dcea26fda0d87
> src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
>
> Diff: https://reviews.apache.org/r/10747/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Change reaper to use an Option as process exit status.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On April 26, 2013, 9:56 p.m., Ben Mahler wrote:
> > src/slave/reaper.cpp, lines 89-91
> > <https://reviews.apache.org/r/10747/diff/2/?file=285045#file285045line89>
> >
> > Move this onto the second line, as it fits in 80 characters:
> >
> > >>> len(' "Failed to monitor process " + stringify(pid) + ": " + strerror(errno));')
> > 80
>
> Jiang Yan Xu wrote:
> Unfortunately it's 81 characters...
>
> Ben Mahler wrote:
> Did I miss a character? It's 80 characters as you have it here, are you looking at the cursor position in eclipse? It will say 81 when on a cursor position on the end of an 80 character line.
Yeah I have python len() calculated it :)
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10747/#review19806
-----------------------------------------------------------
On April 29, 2013, 7:34 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10747/
> -----------------------------------------------------------
>
> (Updated April 29, 2013, 7:34 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolator.hpp f8fabc4e1c3c303b35a76db96b4b2479bd7c8ff8
> src/slave/cgroups_isolator.cpp 8b79da50d8fb0c2c8716dd7d2c734b65c32f60b4
> src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
> src/slave/process_isolator.cpp 6e2af87d291d7c3448393c1ffa816f7020e2dff6
> src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
> src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
> src/slave/slave.hpp f9d9c2b32983700fb09b266a0abf2a396174ae05
> src/slave/slave.cpp 86a15fce93e6aa3f7a8f9700ef2dcea26fda0d87
> src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
>
> Diff: https://reviews.apache.org/r/10747/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Change reaper to use an Option as process exit status.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On April 26, 2013, 9:56 p.m., Ben Mahler wrote:
> > src/slave/cgroups_isolator.cpp, line 822
> > <https://reviews.apache.org/r/10747/diff/2/?file=285041#file285041line822>
> >
> > is stringify needed?
Yes because the ternary operator requires the elements on each side of ':' to be of the same type.
> On April 26, 2013, 9:56 p.m., Ben Mahler wrote:
> > src/slave/reaper.cpp, lines 89-91
> > <https://reviews.apache.org/r/10747/diff/2/?file=285045#file285045line89>
> >
> > Move this onto the second line, as it fits in 80 characters:
> >
> > >>> len(' "Failed to monitor process " + stringify(pid) + ": " + strerror(errno));')
> > 80
Unfortunately it's 81 characters...
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10747/#review19806
-----------------------------------------------------------
On April 29, 2013, 7:34 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10747/
> -----------------------------------------------------------
>
> (Updated April 29, 2013, 7:34 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolator.hpp f8fabc4e1c3c303b35a76db96b4b2479bd7c8ff8
> src/slave/cgroups_isolator.cpp 8b79da50d8fb0c2c8716dd7d2c734b65c32f60b4
> src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
> src/slave/process_isolator.cpp 6e2af87d291d7c3448393c1ffa816f7020e2dff6
> src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
> src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
> src/slave/slave.hpp f9d9c2b32983700fb09b266a0abf2a396174ae05
> src/slave/slave.cpp 86a15fce93e6aa3f7a8f9700ef2dcea26fda0d87
> src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
>
> Diff: https://reviews.apache.org/r/10747/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Change reaper to use an Option as process exit status.
Posted by Ben Mahler <be...@gmail.com>.
> On April 26, 2013, 9:56 p.m., Ben Mahler wrote:
> > src/slave/reaper.cpp, lines 89-91
> > <https://reviews.apache.org/r/10747/diff/2/?file=285045#file285045line89>
> >
> > Move this onto the second line, as it fits in 80 characters:
> >
> > >>> len(' "Failed to monitor process " + stringify(pid) + ": " + strerror(errno));')
> > 80
>
> Jiang Yan Xu wrote:
> Unfortunately it's 81 characters...
Did I miss a character? It's 80 characters as you have it here, are you looking at the cursor position in eclipse? It will say 81 when on a cursor position on the end of an 80 character line.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10747/#review19806
-----------------------------------------------------------
On April 29, 2013, 7:34 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10747/
> -----------------------------------------------------------
>
> (Updated April 29, 2013, 7:34 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolator.hpp f8fabc4e1c3c303b35a76db96b4b2479bd7c8ff8
> src/slave/cgroups_isolator.cpp 8b79da50d8fb0c2c8716dd7d2c734b65c32f60b4
> src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
> src/slave/process_isolator.cpp 6e2af87d291d7c3448393c1ffa816f7020e2dff6
> src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
> src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
> src/slave/slave.hpp f9d9c2b32983700fb09b266a0abf2a396174ae05
> src/slave/slave.cpp 86a15fce93e6aa3f7a8f9700ef2dcea26fda0d87
> src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
>
> Diff: https://reviews.apache.org/r/10747/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Change reaper to use an Option as process exit status.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10747/#review19806
-----------------------------------------------------------
src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/10747/#comment40844>
is stringify needed?
src/slave/reaper.cpp
<https://reviews.apache.org/r/10747/#comment40848>
Move this onto the second line, as it fits in 80 characters:
>>> len(' "Failed to monitor process " + stringify(pid) + ": " + strerror(errno));')
80
- Ben Mahler
On April 26, 2013, 7:57 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10747/
> -----------------------------------------------------------
>
> (Updated April 26, 2013, 7:57 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolator.hpp f8fabc4e1c3c303b35a76db96b4b2479bd7c8ff8
> src/slave/cgroups_isolator.cpp 8b79da50d8fb0c2c8716dd7d2c734b65c32f60b4
> src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
> src/slave/process_isolator.cpp 6e2af87d291d7c3448393c1ffa816f7020e2dff6
> src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
> src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
> src/slave/slave.hpp f9d9c2b32983700fb09b266a0abf2a396174ae05
> src/slave/slave.cpp a0d3864d3e02c7889646f9ff004dad82f67d263b
> src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
>
> Diff: https://reviews.apache.org/r/10747/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Change reaper to use an Option as process exit status.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On April 26, 2013, 10:40 p.m., Vinod Kone wrote:
> > src/slave/cgroups_isolator.cpp, lines 772-774
> > <https://reviews.apache.org/r/10747/diff/1-2/?file=283980#file283980line772>
> >
> > one arg per line.
Sorry overlooked this one...
Addressed in https://reviews.apache.org/r/10746/
> On April 26, 2013, 10:40 p.m., Vinod Kone wrote:
> > src/slave/cgroups_isolator.cpp, lines 818-823
> > <https://reviews.apache.org/r/10747/diff/1-2/?file=283980#file283980line818>
> >
> > This reads different here and in slave.cpp.
> >
> > The reason I wanted you to print status here is because for cgroupsisolator there is a delay between when reaped() is called and when executorTerminated() is sent to the slave. And I wondered if the exit status would be useful if we ever failed to send executorTerminated (e.g., FATAL error in cgroups cleanup).
> >
> > To sum, either we should make the log message consistent or just not print the status. Thoughts?
> >
> >
As discussed offline, we keep it as is.
> On April 26, 2013, 10:40 p.m., Vinod Kone wrote:
> > src/slave/reaper.hpp, lines 62-63
> > <https://reviews.apache.org/r/10747/diff/1-2/?file=283983#file283983line62>
> >
> > s/, / and the process hasn't been reaped yet/ ?
Addressed in https://reviews.apache.org/r/10746/
> On April 26, 2013, 10:40 p.m., Vinod Kone wrote:
> > src/slave/reaper.hpp, line 60
> > <https://reviews.apache.org/r/10747/diff/1-2/?file=283983#file283983line60>
> >
> > s/ a Future in a failed state/failed Future/
Addressed in https://reviews.apache.org/r/10746/
> On April 26, 2013, 10:40 p.m., Vinod Kone wrote:
> > src/slave/reaper.hpp, line 72
> > <https://reviews.apache.org/r/10747/diff/1-2/?file=283983#file283983line72>
> >
> > s/of/in/ ?
> >
> > If you like, you can kill the second sentence altogether.
Addressed in https://reviews.apache.org/r/10746/
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10747/#review19808
-----------------------------------------------------------
On April 29, 2013, 7:34 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10747/
> -----------------------------------------------------------
>
> (Updated April 29, 2013, 7:34 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolator.hpp f8fabc4e1c3c303b35a76db96b4b2479bd7c8ff8
> src/slave/cgroups_isolator.cpp 8b79da50d8fb0c2c8716dd7d2c734b65c32f60b4
> src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
> src/slave/process_isolator.cpp 6e2af87d291d7c3448393c1ffa816f7020e2dff6
> src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
> src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
> src/slave/slave.hpp f9d9c2b32983700fb09b266a0abf2a396174ae05
> src/slave/slave.cpp 86a15fce93e6aa3f7a8f9700ef2dcea26fda0d87
> src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
>
> Diff: https://reviews.apache.org/r/10747/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Change reaper to use an Option as process exit status.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10747/#review19808
-----------------------------------------------------------
src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/10747/#comment40850>
one arg per line.
src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/10747/#comment40851>
How about
if (!status.isReady()) {
LOG(ERROR) << "Failed to get status for " << pid
<< ": " << (status.isFailed() ? status.failure() : "discarded")
}
src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/10747/#comment40865>
This reads different here and in slave.cpp.
The reason I wanted you to print status here is because for cgroupsisolator there is a delay between when reaped() is called and when executorTerminated() is sent to the slave. And I wondered if the exit status would be useful if we ever failed to send executorTerminated (e.g., FATAL error in cgroups cleanup).
To sum, either we should make the log message consistent or just not print the status. Thoughts?
src/slave/process_isolator.cpp
<https://reviews.apache.org/r/10747/#comment40852>
thank you
src/slave/process_isolator.cpp
<https://reviews.apache.org/r/10747/#comment40853>
ditto
src/slave/reaper.hpp
<https://reviews.apache.org/r/10747/#comment40854>
s/ a Future in a failed state/failed Future/
src/slave/reaper.hpp
<https://reviews.apache.org/r/10747/#comment40855>
s/, / and the process hasn't been reaped yet/ ?
src/slave/reaper.hpp
<https://reviews.apache.org/r/10747/#comment40857>
s/of/in/ ?
If you like, you can kill the second sentence altogether.
- Vinod Kone
On April 26, 2013, 7:57 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10747/
> -----------------------------------------------------------
>
> (Updated April 26, 2013, 7:57 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolator.hpp f8fabc4e1c3c303b35a76db96b4b2479bd7c8ff8
> src/slave/cgroups_isolator.cpp 8b79da50d8fb0c2c8716dd7d2c734b65c32f60b4
> src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
> src/slave/process_isolator.cpp 6e2af87d291d7c3448393c1ffa816f7020e2dff6
> src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
> src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
> src/slave/slave.hpp f9d9c2b32983700fb09b266a0abf2a396174ae05
> src/slave/slave.cpp a0d3864d3e02c7889646f9ff004dad82f67d263b
> src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
>
> Diff: https://reviews.apache.org/r/10747/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Change reaper to use an Option as process exit status.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10747/#review19907
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On April 29, 2013, 7:34 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10747/
> -----------------------------------------------------------
>
> (Updated April 29, 2013, 7:34 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolator.hpp f8fabc4e1c3c303b35a76db96b4b2479bd7c8ff8
> src/slave/cgroups_isolator.cpp 8b79da50d8fb0c2c8716dd7d2c734b65c32f60b4
> src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
> src/slave/process_isolator.cpp 6e2af87d291d7c3448393c1ffa816f7020e2dff6
> src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
> src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
> src/slave/slave.hpp f9d9c2b32983700fb09b266a0abf2a396174ae05
> src/slave/slave.cpp 86a15fce93e6aa3f7a8f9700ef2dcea26fda0d87
> src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
>
> Diff: https://reviews.apache.org/r/10747/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Change reaper to use an Option as process exit status.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10747/#review19932
-----------------------------------------------------------
Ship it!
Ship It!
- Ben Mahler
On April 30, 2013, 1:42 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10747/
> -----------------------------------------------------------
>
> (Updated April 30, 2013, 1:42 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolator.hpp f8fabc4e1c3c303b35a76db96b4b2479bd7c8ff8
> src/slave/cgroups_isolator.cpp 8b79da50d8fb0c2c8716dd7d2c734b65c32f60b4
> src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
> src/slave/process_isolator.cpp 6e2af87d291d7c3448393c1ffa816f7020e2dff6
> src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
> src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
> src/slave/slave.hpp f9d9c2b32983700fb09b266a0abf2a396174ae05
> src/slave/slave.cpp 86a15fce93e6aa3f7a8f9700ef2dcea26fda0d87
> src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
>
> Diff: https://reviews.apache.org/r/10747/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Change reaper to use an Option as process exit status.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10747/
-----------------------------------------------------------
(Updated April 30, 2013, 9:03 p.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
Changes
-------
Rebased.
Description
-------
See summary.
Diffs (updated)
-----
src/slave/cgroups_isolator.hpp 7b8270d0b0f94a71da12bc123d39b44c40c3f7ed
src/slave/cgroups_isolator.cpp f12fd48fe739015429a3fe51cc8dce366ec4a483
src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
src/slave/process_isolator.cpp 6938fbcca9d384bb014ff4dd52a13763c1f8397a
src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
src/slave/reaper.cpp b4d8912fa4a5910b0003c06b87a7fbc6164a6382
src/slave/slave.hpp f9d9c2b32983700fb09b266a0abf2a396174ae05
src/slave/slave.cpp 86a15fce93e6aa3f7a8f9700ef2dcea26fda0d87
src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
Diff: https://reviews.apache.org/r/10747/diff/
Testing
-------
make check.
Thanks,
Jiang Yan Xu
Re: Review Request: Change reaper to use an Option as process exit status.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10747/
-----------------------------------------------------------
(Updated April 30, 2013, 5:57 a.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
Changes
-------
Fixed an indentation issue.
Description
-------
See summary.
Diffs (updated)
-----
src/slave/cgroups_isolator.hpp 7b8270d0b0f94a71da12bc123d39b44c40c3f7ed
src/slave/cgroups_isolator.cpp f12fd48fe739015429a3fe51cc8dce366ec4a483
src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
src/slave/process_isolator.cpp 6938fbcca9d384bb014ff4dd52a13763c1f8397a
src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
src/slave/reaper.cpp b4d8912fa4a5910b0003c06b87a7fbc6164a6382
src/slave/slave.hpp f9d9c2b32983700fb09b266a0abf2a396174ae05
src/slave/slave.cpp 86a15fce93e6aa3f7a8f9700ef2dcea26fda0d87
src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
Diff: https://reviews.apache.org/r/10747/diff/
Testing
-------
make check.
Thanks,
Jiang Yan Xu
Re: Review Request: Change reaper to use an Option as process exit status.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10747/
-----------------------------------------------------------
(Updated April 30, 2013, 1:42 a.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
Changes
-------
NONE -> 'None' in comments.
Description
-------
See summary.
Diffs (updated)
-----
src/slave/cgroups_isolator.hpp f8fabc4e1c3c303b35a76db96b4b2479bd7c8ff8
src/slave/cgroups_isolator.cpp 8b79da50d8fb0c2c8716dd7d2c734b65c32f60b4
src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
src/slave/process_isolator.cpp 6e2af87d291d7c3448393c1ffa816f7020e2dff6
src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
src/slave/slave.hpp f9d9c2b32983700fb09b266a0abf2a396174ae05
src/slave/slave.cpp 86a15fce93e6aa3f7a8f9700ef2dcea26fda0d87
src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
Diff: https://reviews.apache.org/r/10747/diff/
Testing
-------
make check.
Thanks,
Jiang Yan Xu
Re: Review Request: Change reaper to use an Option as process exit status.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10747/#review19929
-----------------------------------------------------------
Ship it!
src/slave/reaper.cpp
<https://reviews.apache.org/r/10747/#comment41097>
nice!
src/slave/reaper.hpp
<https://reviews.apache.org/r/10747/#comment41093>
s/NONE/'None'/
src/slave/reaper.hpp
<https://reviews.apache.org/r/10747/#comment41094>
ditto
src/slave/reaper.cpp
<https://reviews.apache.org/r/10747/#comment41095>
ditto
- Vinod Kone
On April 30, 2013, 1:26 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10747/
> -----------------------------------------------------------
>
> (Updated April 30, 2013, 1:26 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolator.hpp f8fabc4e1c3c303b35a76db96b4b2479bd7c8ff8
> src/slave/cgroups_isolator.cpp 8b79da50d8fb0c2c8716dd7d2c734b65c32f60b4
> src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
> src/slave/process_isolator.cpp 6e2af87d291d7c3448393c1ffa816f7020e2dff6
> src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
> src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
> src/slave/slave.hpp f9d9c2b32983700fb09b266a0abf2a396174ae05
> src/slave/slave.cpp 86a15fce93e6aa3f7a8f9700ef2dcea26fda0d87
> src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
>
> Diff: https://reviews.apache.org/r/10747/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Change reaper to use an Option as process exit status.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10747/
-----------------------------------------------------------
(Updated April 30, 2013, 1:26 a.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
Changes
-------
Minor change because of the immediate return of monitor() for invalid pids. "-1" -> "None()".
Description
-------
See summary.
Diffs (updated)
-----
src/slave/cgroups_isolator.hpp f8fabc4e1c3c303b35a76db96b4b2479bd7c8ff8
src/slave/cgroups_isolator.cpp 8b79da50d8fb0c2c8716dd7d2c734b65c32f60b4
src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
src/slave/process_isolator.cpp 6e2af87d291d7c3448393c1ffa816f7020e2dff6
src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
src/slave/slave.hpp f9d9c2b32983700fb09b266a0abf2a396174ae05
src/slave/slave.cpp 86a15fce93e6aa3f7a8f9700ef2dcea26fda0d87
src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
Diff: https://reviews.apache.org/r/10747/diff/
Testing
-------
make check.
Thanks,
Jiang Yan Xu
Re: Review Request: Change reaper to use an Option as process exit status.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10747/
-----------------------------------------------------------
(Updated April 29, 2013, 7:34 a.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
Changes
-------
Addressed comments.
Description
-------
See summary.
Diffs (updated)
-----
src/slave/cgroups_isolator.hpp f8fabc4e1c3c303b35a76db96b4b2479bd7c8ff8
src/slave/cgroups_isolator.cpp 8b79da50d8fb0c2c8716dd7d2c734b65c32f60b4
src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
src/slave/process_isolator.cpp 6e2af87d291d7c3448393c1ffa816f7020e2dff6
src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
src/slave/slave.hpp f9d9c2b32983700fb09b266a0abf2a396174ae05
src/slave/slave.cpp 86a15fce93e6aa3f7a8f9700ef2dcea26fda0d87
src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
Diff: https://reviews.apache.org/r/10747/diff/
Testing
-------
make check.
Thanks,
Jiang Yan Xu
Re: Review Request: Change reaper to use an Option as process exit status.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10747/
-----------------------------------------------------------
(Updated April 26, 2013, 7:57 p.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
Changes
-------
Addressed comments.
Description
-------
See summary.
Diffs (updated)
-----
src/slave/cgroups_isolator.hpp f8fabc4e1c3c303b35a76db96b4b2479bd7c8ff8
src/slave/cgroups_isolator.cpp 8b79da50d8fb0c2c8716dd7d2c734b65c32f60b4
src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
src/slave/process_isolator.cpp 6e2af87d291d7c3448393c1ffa816f7020e2dff6
src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
src/slave/slave.hpp f9d9c2b32983700fb09b266a0abf2a396174ae05
src/slave/slave.cpp a0d3864d3e02c7889646f9ff004dad82f67d263b
src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
Diff: https://reviews.apache.org/r/10747/diff/
Testing
-------
make check.
Thanks,
Jiang Yan Xu
Re: Review Request: Change reaper to use an Option as process exit status.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On April 24, 2013, 8:30 p.m., Benjamin Hindman wrote:
> > src/slave/slave.cpp, line 2041
> > <https://reviews.apache.org/r/10747/diff/1/?file=283986#file283986line2041>
> >
> > The compiler will create the temps for you. ;)
Can't, due to error.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10747/#review19652
-----------------------------------------------------------
On April 26, 2013, 7:57 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10747/
> -----------------------------------------------------------
>
> (Updated April 26, 2013, 7:57 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolator.hpp f8fabc4e1c3c303b35a76db96b4b2479bd7c8ff8
> src/slave/cgroups_isolator.cpp 8b79da50d8fb0c2c8716dd7d2c734b65c32f60b4
> src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
> src/slave/process_isolator.cpp 6e2af87d291d7c3448393c1ffa816f7020e2dff6
> src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
> src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
> src/slave/slave.hpp f9d9c2b32983700fb09b266a0abf2a396174ae05
> src/slave/slave.cpp a0d3864d3e02c7889646f9ff004dad82f67d263b
> src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
>
> Diff: https://reviews.apache.org/r/10747/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Change reaper to use an Option as process exit status.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10747/#review19652
-----------------------------------------------------------
src/slave/reaper.cpp
<https://reviews.apache.org/r/10747/#comment40597>
Pull the notify calls into the if and else blocks to make the None case extremely explicit:
int status = -1;
if (waitpid(pid, &status, WNOHANG) < 0) {
LOG(WARNING) << ...;
notify(pid, None());
} else {
notify(pid, status);
}
statuses.remove(pid);
src/slave/slave.cpp
<https://reviews.apache.org/r/10747/#comment40598>
The compiler will create the temps for you. ;)
- Benjamin Hindman
On April 24, 2013, 12:51 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10747/
> -----------------------------------------------------------
>
> (Updated April 24, 2013, 12:51 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolator.hpp f8fabc4e1c3c303b35a76db96b4b2479bd7c8ff8
> src/slave/cgroups_isolator.cpp 8b79da50d8fb0c2c8716dd7d2c734b65c32f60b4
> src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
> src/slave/process_isolator.cpp 6e2af87d291d7c3448393c1ffa816f7020e2dff6
> src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
> src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
> src/slave/slave.hpp f9d9c2b32983700fb09b266a0abf2a396174ae05
> src/slave/slave.cpp a0d3864d3e02c7889646f9ff004dad82f67d263b
> src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
>
> Diff: https://reviews.apache.org/r/10747/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Change reaper to use an Option as process exit status.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On April 24, 2013, 7:45 p.m., Vinod Kone wrote:
> > src/tests/reaper_tests.cpp, line 173
> > <https://reviews.apache.org/r/10747/diff/1/?file=283987#file283987line173>
> >
> > no need for temp
The problem I have having is that the following usage
ASSERT_TRUE(WIFSIGNALED(processExited.get().get()));
causes this compilation error:
../../src/tests/reaper_tests.cpp: In member function ‘virtual void ReaperTest_ChildProcess_Test::TestBody()’:
../../src/tests/reaper_tests.cpp:174: error: invalid lvalue in unary ‘&’
It cannot be solved using more parentheses...
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10747/#review19643
-----------------------------------------------------------
On April 24, 2013, 12:51 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10747/
> -----------------------------------------------------------
>
> (Updated April 24, 2013, 12:51 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolator.hpp f8fabc4e1c3c303b35a76db96b4b2479bd7c8ff8
> src/slave/cgroups_isolator.cpp 8b79da50d8fb0c2c8716dd7d2c734b65c32f60b4
> src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
> src/slave/process_isolator.cpp 6e2af87d291d7c3448393c1ffa816f7020e2dff6
> src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
> src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
> src/slave/slave.hpp f9d9c2b32983700fb09b266a0abf2a396174ae05
> src/slave/slave.cpp a0d3864d3e02c7889646f9ff004dad82f67d263b
> src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
>
> Diff: https://reviews.apache.org/r/10747/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Change reaper to use an Option as process exit status.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On April 24, 2013, 7:45 p.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 2041
> > <https://reviews.apache.org/r/10747/diff/1/?file=283986#file283986line2041>
> >
> > just use status.get() instead of storing in a temp.
This is again because the use WIFEXITED macro with status.get() causes Error: invalid lvalue in unary '&'. Changed stat to status_ as BenH suggested.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10747/#review19643
-----------------------------------------------------------
On April 26, 2013, 7:57 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10747/
> -----------------------------------------------------------
>
> (Updated April 26, 2013, 7:57 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolator.hpp f8fabc4e1c3c303b35a76db96b4b2479bd7c8ff8
> src/slave/cgroups_isolator.cpp 8b79da50d8fb0c2c8716dd7d2c734b65c32f60b4
> src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
> src/slave/process_isolator.cpp 6e2af87d291d7c3448393c1ffa816f7020e2dff6
> src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
> src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
> src/slave/slave.hpp f9d9c2b32983700fb09b266a0abf2a396174ae05
> src/slave/slave.cpp a0d3864d3e02c7889646f9ff004dad82f67d263b
> src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
>
> Diff: https://reviews.apache.org/r/10747/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Change reaper to use an Option as process exit status.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10747/#review19643
-----------------------------------------------------------
src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/10747/#comment40582>
why not print the status!
how about:
<< "terminated with status " << (status.isSome() ? status : "unknown");
src/slave/reaper.hpp
<https://reviews.apache.org/r/10747/#comment40583>
you need to update this comment
src/slave/slave.cpp
<https://reviews.apache.org/r/10747/#comment40584>
just use status.get() instead of storing in a temp.
src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/10747/#comment40585>
no need for temp
- Vinod Kone
On April 24, 2013, 12:51 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10747/
> -----------------------------------------------------------
>
> (Updated April 24, 2013, 12:51 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolator.hpp f8fabc4e1c3c303b35a76db96b4b2479bd7c8ff8
> src/slave/cgroups_isolator.cpp 8b79da50d8fb0c2c8716dd7d2c734b65c32f60b4
> src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
> src/slave/process_isolator.cpp 6e2af87d291d7c3448393c1ffa816f7020e2dff6
> src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
> src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
> src/slave/slave.hpp f9d9c2b32983700fb09b266a0abf2a396174ae05
> src/slave/slave.cpp a0d3864d3e02c7889646f9ff004dad82f67d263b
> src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
>
> Diff: https://reviews.apache.org/r/10747/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>