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/26 21:51:50 UTC
Re: Review Request: Moved proc::alive function to os::alive in os.hpp and
improved its documentation. (libstout)
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10745/
-----------------------------------------------------------
(Updated April 26, 2013, 7:51 p.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
Changes
-------
Addressed comments.
Summary (updated)
-----------------
Moved proc::alive function to os::alive in os.hpp and improved its documentation. (libstout)
Description
-------
See summary.
Diffs (updated)
-----
src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
third_party/libprocess/third_party/stout/include/stout/os.hpp 4d51693504f15236c131600737d0b6ddb6a1a819
third_party/libprocess/third_party/stout/include/stout/proc.hpp 19000eb182cef4ecbf10fc3aa6c6e6c076f1ac46
Diff: https://reviews.apache.org/r/10745/diff/
Testing
-------
N/A
Thanks,
Jiang Yan Xu
Re: Review Request: Moved proc::alive function to os::alive in os.hpp and
improved its documentation. (libstout)
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On April 29, 2013, 9:19 p.m., Vinod Kone wrote:
> > third_party/libprocess/third_party/stout/include/stout/os.hpp, line 1042
> > <https://reviews.apache.org/r/10745/diff/2-3/?file=285028#file285028line1042>
> >
> > I think it reads better as "A process is not considered alive iff it is terminated AND reaped". Thoughts?
Yeah I think this is better. Confusing terms...
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10745/#review19893
-----------------------------------------------------------
On April 30, 2013, 1:16 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10745/
> -----------------------------------------------------------
>
> (Updated April 30, 2013, 1:16 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
> third_party/libprocess/third_party/stout/include/stout/os.hpp 4d51693504f15236c131600737d0b6ddb6a1a819
> third_party/libprocess/third_party/stout/include/stout/proc.hpp 19000eb182cef4ecbf10fc3aa6c6e6c076f1ac46
>
> Diff: https://reviews.apache.org/r/10745/diff/
>
>
> Testing
> -------
>
> N/A
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Moved proc::alive function to os::alive in os.hpp and
improved its documentation. (libstout)
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10745/#review19893
-----------------------------------------------------------
Ship it!
third_party/libprocess/third_party/stout/include/stout/os.hpp
<https://reviews.apache.org/r/10745/#comment41037>
I think it reads better as "A process is not considered alive iff it is terminated AND reaped". Thoughts?
- Vinod Kone
On April 29, 2013, 7:30 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10745/
> -----------------------------------------------------------
>
> (Updated April 29, 2013, 7:30 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
> third_party/libprocess/third_party/stout/include/stout/os.hpp 4d51693504f15236c131600737d0b6ddb6a1a819
> third_party/libprocess/third_party/stout/include/stout/proc.hpp 19000eb182cef4ecbf10fc3aa6c6e6c076f1ac46
>
> Diff: https://reviews.apache.org/r/10745/diff/
>
>
> Testing
> -------
>
> N/A
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Moved proc::alive function to os::alive in os.hpp and
improved its documentation. (libstout)
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10745/#review19875
-----------------------------------------------------------
third_party/libprocess/third_party/stout/include/stout/os.hpp
<https://reviews.apache.org/r/10745/#comment40993>
Not yours, but can you change this to use Error() instead to guard against -1? We don't use glog in stout files (although there are some remaining instances, like this one).
- Ben Mahler
On April 29, 2013, 7:30 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10745/
> -----------------------------------------------------------
>
> (Updated April 29, 2013, 7:30 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
> third_party/libprocess/third_party/stout/include/stout/os.hpp 4d51693504f15236c131600737d0b6ddb6a1a819
> third_party/libprocess/third_party/stout/include/stout/proc.hpp 19000eb182cef4ecbf10fc3aa6c6e6c076f1ac46
>
> Diff: https://reviews.apache.org/r/10745/diff/
>
>
> Testing
> -------
>
> N/A
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Moved proc::alive function to os::alive in os.hpp and
improved its documentation. (libstout)
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10745/#review19928
-----------------------------------------------------------
Ship it!
third_party/libprocess/third_party/stout/include/stout/os.hpp
<https://reviews.apache.org/r/10745/#comment41092>
:) you'll later maybe want to test this case in proc_tests.cpp
- Ben Mahler
On April 30, 2013, 1:16 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10745/
> -----------------------------------------------------------
>
> (Updated April 30, 2013, 1:16 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
> third_party/libprocess/third_party/stout/include/stout/os.hpp 4d51693504f15236c131600737d0b6ddb6a1a819
> third_party/libprocess/third_party/stout/include/stout/proc.hpp 19000eb182cef4ecbf10fc3aa6c6e6c076f1ac46
>
> Diff: https://reviews.apache.org/r/10745/diff/
>
>
> Testing
> -------
>
> N/A
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Moved proc::alive function to os::alive in os.hpp and
improved its documentation. (libstout)
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10745/
-----------------------------------------------------------
(Updated April 30, 2013, 5:53 a.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
Changes
-------
Rebased. Removed reaper.cpp and proc.cpp from the commit because the trunk already has the change.
Description
-------
See summary.
Diffs (updated)
-----
third_party/libprocess/third_party/stout/include/stout/os.hpp c71ae9ed32af2a14dc7b9b34521cb5ea6025235c
Diff: https://reviews.apache.org/r/10745/diff/
Testing
-------
N/A
Thanks,
Jiang Yan Xu
Re: Review Request: Moved proc::alive function to os::alive in os.hpp and
improved its documentation. (libstout)
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10745/
-----------------------------------------------------------
(Updated April 30, 2013, 1:16 a.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
Changes
-------
Addressed comments.
Description
-------
See summary.
Diffs (updated)
-----
src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
third_party/libprocess/third_party/stout/include/stout/os.hpp 4d51693504f15236c131600737d0b6ddb6a1a819
third_party/libprocess/third_party/stout/include/stout/proc.hpp 19000eb182cef4ecbf10fc3aa6c6e6c076f1ac46
Diff: https://reviews.apache.org/r/10745/diff/
Testing
-------
N/A
Thanks,
Jiang Yan Xu
Re: Review Request: Moved proc::alive function to os::alive in os.hpp and
improved its documentation. (libstout)
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10745/
-----------------------------------------------------------
(Updated April 29, 2013, 7:30 a.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
Changes
-------
Addressed comments.
Description
-------
See summary.
Diffs (updated)
-----
src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
third_party/libprocess/third_party/stout/include/stout/os.hpp 4d51693504f15236c131600737d0b6ddb6a1a819
third_party/libprocess/third_party/stout/include/stout/proc.hpp 19000eb182cef4ecbf10fc3aa6c6e6c076f1ac46
Diff: https://reviews.apache.org/r/10745/diff/
Testing
-------
N/A
Thanks,
Jiang Yan Xu
Re: Review Request: Moved proc::alive function to os::alive in os.hpp and
improved its documentation. (libstout)
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On April 26, 2013, 10:08 p.m., Ben Mahler wrote:
> > Can you add the relevant TODOs to use /proc for more reliable os::alive checking on linux?
Added. I agree that reaper and os::alive can both improve with the help of /proc info.
> On April 26, 2013, 10:08 p.m., Ben Mahler wrote:
> > third_party/libprocess/third_party/stout/include/stout/os.hpp, lines 1044-1045
> > <https://reviews.apache.org/r/10745/diff/2/?file=285028#file285028line1044>
> >
> > 2 can be verified, but not 1? Is that why this function does not check these?
1) and 2) are the same thing, the open group doc says about permission: "unless the sending process has appropriate privileges, the real or effective user ID of the sending process shall match the real or saved set-user-ID of the receiving process."
I modified 1) to better match the open group doc.
> On April 26, 2013, 10:08 p.m., Ben Mahler wrote:
> > third_party/libprocess/third_party/stout/include/stout/os.hpp, line 1070
> > <https://reviews.apache.org/r/10745/diff/2/?file=285028#file285028line1070>
> >
> > s/Try<bool>::error/Error/
> >
> > Be sure to include <stout/error.hpp>
Changed to ErrnoError(). Thanks.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10745/#review19809
-----------------------------------------------------------
On April 29, 2013, 7:30 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10745/
> -----------------------------------------------------------
>
> (Updated April 29, 2013, 7:30 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
> third_party/libprocess/third_party/stout/include/stout/os.hpp 4d51693504f15236c131600737d0b6ddb6a1a819
> third_party/libprocess/third_party/stout/include/stout/proc.hpp 19000eb182cef4ecbf10fc3aa6c6e6c076f1ac46
>
> Diff: https://reviews.apache.org/r/10745/diff/
>
>
> Testing
> -------
>
> N/A
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Moved proc::alive function to os::alive in os.hpp and
improved its documentation. (libstout)
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10745/#review19809
-----------------------------------------------------------
Can you add the relevant TODOs to use /proc for more reliable os::alive checking on linux?
third_party/libprocess/third_party/stout/include/stout/os.hpp
<https://reviews.apache.org/r/10745/#comment40858>
2 can be verified, but not 1? Is that why this function does not check these?
third_party/libprocess/third_party/stout/include/stout/os.hpp
<https://reviews.apache.org/r/10745/#comment40859>
s/Try<bool>::error/Error/
Be sure to include <stout/error.hpp>
third_party/libprocess/third_party/stout/include/stout/proc.hpp
<https://reviews.apache.org/r/10745/#comment40856>
You can make a TODO for me here, as I'm currently working on this :)
Also, it will be for /proc utilities, not for process utilities.
- Ben Mahler
On April 26, 2013, 7:51 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10745/
> -----------------------------------------------------------
>
> (Updated April 26, 2013, 7:51 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
> third_party/libprocess/third_party/stout/include/stout/os.hpp 4d51693504f15236c131600737d0b6ddb6a1a819
> third_party/libprocess/third_party/stout/include/stout/proc.hpp 19000eb182cef4ecbf10fc3aa6c6e6c076f1ac46
>
> Diff: https://reviews.apache.org/r/10745/diff/
>
>
> Testing
> -------
>
> N/A
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request: Moved proc::alive function to os::alive in os.hpp and
improved its documentation. (libstout)
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10745/#review19805
-----------------------------------------------------------
third_party/libprocess/third_party/stout/include/stout/proc.hpp
<https://reviews.apache.org/r/10745/#comment40847>
Make it a TODO.
third_party/libprocess/third_party/stout/include/stout/os.hpp
<https://reviews.apache.org/r/10745/#comment40842>
what does ''inactive' mean?
Lets just say "A process is considered alive iff it is not terminated and reaped"
third_party/libprocess/third_party/stout/include/stout/os.hpp
<https://reviews.apache.org/r/10745/#comment40843>
s/Note/NOTE/
third_party/libprocess/third_party/stout/include/stout/os.hpp
<https://reviews.apache.org/r/10745/#comment40845>
Why is this on a new line?
- Vinod Kone
On April 26, 2013, 7:51 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10745/
> -----------------------------------------------------------
>
> (Updated April 26, 2013, 7:51 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3
> third_party/libprocess/third_party/stout/include/stout/os.hpp 4d51693504f15236c131600737d0b6ddb6a1a819
> third_party/libprocess/third_party/stout/include/stout/proc.hpp 19000eb182cef4ecbf10fc3aa6c6e6c076f1ac46
>
> Diff: https://reviews.apache.org/r/10745/diff/
>
>
> Testing
> -------
>
> N/A
>
>
> Thanks,
>
> Jiang Yan Xu
>
>