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/05/31 01:11:04 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/11556/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
Description
-------
Rebased against master. Previous review received "shipit"s but got lost.
Diffs
-----
src/slave/cgroups_isolator.hpp 7b8270d0b0f94a71da12bc123d39b44c40c3f7ed
src/slave/cgroups_isolator.cpp 9b3a3a5dfec27a119fdd47a88f016e21470eb88d
src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
src/slave/process_isolator.cpp d4f7b7603452e7226189899aca7e3c94e537829f
src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
src/slave/reaper.cpp 1d826d1b5fe1048b03662a1e9d12b18371e5c88f
src/slave/slave.hpp 26dc96e5f2fdc0711fc49a9ea80b7f037bf93c29
src/slave/slave.cpp e905ab3cd965002592c1f70c85ea3378ac8982b0
src/tests/reaper_tests.cpp fbb6066652c0f36c0a8fd9b0c4dac7e0240a5210
Diff: https://reviews.apache.org/r/11556/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 May 31, 2013, 5:19 p.m., Vinod Kone wrote:
> > src/slave/reaper.hpp, lines 64-65
> > <https://reviews.apache.org/r/11556/diff/1/?file=299213#file299213line64>
> >
> > why not failed future?
A failed future is returned in the case where we don't have permission to ("failed" to) monitor the pid, as documented in the paragraph above.
An invalid pid results in None() to differentiate itself from the case above and the client code handles them differently.
> On May 31, 2013, 5:19 p.m., Vinod Kone wrote:
> > src/slave/cgroups_isolator.cpp, lines 791-793
> > <https://reviews.apache.org/r/11556/diff/1/?file=299210#file299210line791>
> >
> > Aha. I see that you changed the comment here. Ignore my comment in the previous review.
Pulled it down to include executorId and frameworkId in the logging message.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11556/#review21259
-----------------------------------------------------------
On May 31, 2013, 6:45 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11556/
> -----------------------------------------------------------
>
> (Updated May 31, 2013, 6:45 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> Rebased against master. Previous review received "shipit"s but got lost.
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolator.hpp 7b8270d0b0f94a71da12bc123d39b44c40c3f7ed
> src/slave/cgroups_isolator.cpp 9b3a3a5dfec27a119fdd47a88f016e21470eb88d
> src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
> src/slave/process_isolator.cpp d4f7b7603452e7226189899aca7e3c94e537829f
> src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
> src/slave/reaper.cpp 1d826d1b5fe1048b03662a1e9d12b18371e5c88f
> src/slave/slave.hpp 26dc96e5f2fdc0711fc49a9ea80b7f037bf93c29
> src/slave/slave.cpp e905ab3cd965002592c1f70c85ea3378ac8982b0
> src/tests/reaper_tests.cpp fbb6066652c0f36c0a8fd9b0c4dac7e0240a5210
>
> Diff: https://reviews.apache.org/r/11556/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/11556/#review21259
-----------------------------------------------------------
Ship it!
src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/11556/#comment44133>
Aha. I see that you changed the comment here. Ignore my comment in the previous review.
src/slave/reaper.hpp
<https://reviews.apache.org/r/11556/#comment44134>
why not failed future?
src/slave/slave.cpp
<https://reviews.apache.org/r/11556/#comment44135>
Hmm.. Not sure I like changing the argument.
Can you make arg const Option<int> status and use local variable "status_"?
- Vinod Kone
On May 30, 2013, 11:11 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11556/
> -----------------------------------------------------------
>
> (Updated May 30, 2013, 11:11 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> Rebased against master. Previous review received "shipit"s but got lost.
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolator.hpp 7b8270d0b0f94a71da12bc123d39b44c40c3f7ed
> src/slave/cgroups_isolator.cpp 9b3a3a5dfec27a119fdd47a88f016e21470eb88d
> src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
> src/slave/process_isolator.cpp d4f7b7603452e7226189899aca7e3c94e537829f
> src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
> src/slave/reaper.cpp 1d826d1b5fe1048b03662a1e9d12b18371e5c88f
> src/slave/slave.hpp 26dc96e5f2fdc0711fc49a9ea80b7f037bf93c29
> src/slave/slave.cpp e905ab3cd965002592c1f70c85ea3378ac8982b0
> src/tests/reaper_tests.cpp fbb6066652c0f36c0a8fd9b0c4dac7e0240a5210
>
> Diff: https://reviews.apache.org/r/11556/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/11556/#review21263
-----------------------------------------------------------
src/slave/process_isolator.cpp
<https://reviews.apache.org/r/11556/#comment44140>
why not pull this down too, to include executor and framework ids?
- Vinod Kone
On May 31, 2013, 6:45 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11556/
> -----------------------------------------------------------
>
> (Updated May 31, 2013, 6:45 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> Rebased against master. Previous review received "shipit"s but got lost.
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolator.hpp 7b8270d0b0f94a71da12bc123d39b44c40c3f7ed
> src/slave/cgroups_isolator.cpp 9b3a3a5dfec27a119fdd47a88f016e21470eb88d
> src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
> src/slave/process_isolator.cpp d4f7b7603452e7226189899aca7e3c94e537829f
> src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
> src/slave/reaper.cpp 1d826d1b5fe1048b03662a1e9d12b18371e5c88f
> src/slave/slave.hpp 26dc96e5f2fdc0711fc49a9ea80b7f037bf93c29
> src/slave/slave.cpp e905ab3cd965002592c1f70c85ea3378ac8982b0
> src/tests/reaper_tests.cpp fbb6066652c0f36c0a8fd9b0c4dac7e0240a5210
>
> Diff: https://reviews.apache.org/r/11556/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/11556/
-----------------------------------------------------------
(Updated May 31, 2013, 8:59 p.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
Changes
-------
Fixed typos...
Description
-------
Rebased against master. Previous review received "shipit"s but got lost.
Diffs (updated)
-----
src/slave/cgroups_isolator.hpp 7b8270d0b0f94a71da12bc123d39b44c40c3f7ed
src/slave/cgroups_isolator.cpp 9b3a3a5dfec27a119fdd47a88f016e21470eb88d
src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
src/slave/process_isolator.cpp d4f7b7603452e7226189899aca7e3c94e537829f
src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
src/slave/reaper.cpp 1d826d1b5fe1048b03662a1e9d12b18371e5c88f
src/slave/slave.hpp 26dc96e5f2fdc0711fc49a9ea80b7f037bf93c29
src/slave/slave.cpp e905ab3cd965002592c1f70c85ea3378ac8982b0
src/tests/reaper_tests.cpp fbb6066652c0f36c0a8fd9b0c4dac7e0240a5210
Diff: https://reviews.apache.org/r/11556/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/11556/
-----------------------------------------------------------
(Updated May 31, 2013, 8:57 p.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
Changes
-------
Vinod's comments.
Description
-------
Rebased against master. Previous review received "shipit"s but got lost.
Diffs (updated)
-----
src/slave/cgroups_isolator.hpp 7b8270d0b0f94a71da12bc123d39b44c40c3f7ed
src/slave/cgroups_isolator.cpp 9b3a3a5dfec27a119fdd47a88f016e21470eb88d
src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
src/slave/process_isolator.cpp d4f7b7603452e7226189899aca7e3c94e537829f
src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
src/slave/reaper.cpp 1d826d1b5fe1048b03662a1e9d12b18371e5c88f
src/slave/slave.hpp 26dc96e5f2fdc0711fc49a9ea80b7f037bf93c29
src/slave/slave.cpp e905ab3cd965002592c1f70c85ea3378ac8982b0
src/tests/reaper_tests.cpp fbb6066652c0f36c0a8fd9b0c4dac7e0240a5210
Diff: https://reviews.apache.org/r/11556/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/11556/#review21264
-----------------------------------------------------------
Ship it!
src/slave/slave.cpp
<https://reviews.apache.org/r/11556/#comment44141>
s/status/taskState/ in this for loop too?
src/slave/slave.cpp
<https://reviews.apache.org/r/11556/#comment44142>
thank you
- Vinod Kone
On May 31, 2013, 6:45 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11556/
> -----------------------------------------------------------
>
> (Updated May 31, 2013, 6:45 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> Rebased against master. Previous review received "shipit"s but got lost.
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolator.hpp 7b8270d0b0f94a71da12bc123d39b44c40c3f7ed
> src/slave/cgroups_isolator.cpp 9b3a3a5dfec27a119fdd47a88f016e21470eb88d
> src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
> src/slave/process_isolator.cpp d4f7b7603452e7226189899aca7e3c94e537829f
> src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
> src/slave/reaper.cpp 1d826d1b5fe1048b03662a1e9d12b18371e5c88f
> src/slave/slave.hpp 26dc96e5f2fdc0711fc49a9ea80b7f037bf93c29
> src/slave/slave.cpp e905ab3cd965002592c1f70c85ea3378ac8982b0
> src/tests/reaper_tests.cpp fbb6066652c0f36c0a8fd9b0c4dac7e0240a5210
>
> Diff: https://reviews.apache.org/r/11556/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/11556/
-----------------------------------------------------------
(Updated May 31, 2013, 6:45 p.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
Changes
-------
Addressed Vinod's comments.
Description
-------
Rebased against master. Previous review received "shipit"s but got lost.
Diffs (updated)
-----
src/slave/cgroups_isolator.hpp 7b8270d0b0f94a71da12bc123d39b44c40c3f7ed
src/slave/cgroups_isolator.cpp 9b3a3a5dfec27a119fdd47a88f016e21470eb88d
src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190
src/slave/process_isolator.cpp d4f7b7603452e7226189899aca7e3c94e537829f
src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90
src/slave/reaper.cpp 1d826d1b5fe1048b03662a1e9d12b18371e5c88f
src/slave/slave.hpp 26dc96e5f2fdc0711fc49a9ea80b7f037bf93c29
src/slave/slave.cpp e905ab3cd965002592c1f70c85ea3378ac8982b0
src/tests/reaper_tests.cpp fbb6066652c0f36c0a8fd9b0c4dac7e0240a5210
Diff: https://reviews.apache.org/r/11556/diff/
Testing
-------
make check.
Thanks,
Jiang Yan Xu