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 2013/03/07 04:16:43 UTC
Review Request: Added proc::children for linux.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9794/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Vinod Kone.
Description
-------
This is used in the following review for process based isolation resource usage.
Diffs
-----
src/linux/proc.hpp e0825a4a9f9e2763e0c25d7319f220bfe7c7c29c
src/linux/proc.cpp da30e9a860ba7a142015baf05feaa508bdcdfed5
src/tests/proc_tests.cpp c3fa526dc2909ad7cb84a3a40c5850885b0065e0
Diff: https://reviews.apache.org/r/9794/diff/
Testing
-------
Added tests.
Thanks,
Ben Mahler
Re: Review Request: Added proc::children for linux.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9794/
-----------------------------------------------------------
(Updated March 28, 2013, 9:26 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Rebased off trunk, no need for review.
Description
-------
This is used in the following review for process based isolation resource usage.
Diffs (updated)
-----
src/linux/cgroups.cpp f3dcf9c6f7fbec2ec37921ba0f4f58f5c57387c2
src/linux/proc.hpp f01693eb232938d29cbbb66937b49fa68e74c904
src/linux/proc.cpp 41ecbaf444e92f73714c8933386c6c293b975167
src/tests/proc_tests.cpp c3fa526dc2909ad7cb84a3a40c5850885b0065e0
Diff: https://reviews.apache.org/r/9794/diff/
Testing
-------
Added tests.
Thanks,
Ben Mahler
Re: Review Request: Added proc::children for linux.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9794/
-----------------------------------------------------------
(Updated March 12, 2013, 9:35 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Rebased off trunk, no need for review.
Description
-------
This is used in the following review for process based isolation resource usage.
Diffs (updated)
-----
src/linux/cgroups.cpp 480ae228a1365afa60c9f88c1f31f48e3a5eb294
src/linux/proc.hpp e0825a4a9f9e2763e0c25d7319f220bfe7c7c29c
src/linux/proc.cpp da30e9a860ba7a142015baf05feaa508bdcdfed5
src/tests/proc_tests.cpp c3fa526dc2909ad7cb84a3a40c5850885b0065e0
Diff: https://reviews.apache.org/r/9794/diff/
Testing
-------
Added tests.
Thanks,
Ben Mahler
Re: Review Request: Added proc::children for linux.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9794/
-----------------------------------------------------------
(Updated March 8, 2013, 2:25 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Reviews, refactoring {Process|System}Statistics -> {Process|System}Status, rebased off trunk.
Description
-------
This is used in the following review for process based isolation resource usage.
Diffs (updated)
-----
src/linux/cgroups.cpp edce52475eb58f2aa8f1c99bcaf3cb269167fc1e
src/linux/proc.hpp e0825a4a9f9e2763e0c25d7319f220bfe7c7c29c
src/linux/proc.cpp da30e9a860ba7a142015baf05feaa508bdcdfed5
src/tests/proc_tests.cpp c3fa526dc2909ad7cb84a3a40c5850885b0065e0
Diff: https://reviews.apache.org/r/9794/diff/
Testing
-------
Added tests.
Thanks,
Ben Mahler
Re: Review Request: Added proc::children for linux.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9794/#review17554
-----------------------------------------------------------
Ship it!
src/linux/proc.cpp
<https://reviews.apache.org/r/9794/#comment37213>
I actually prefer 'descendants', because children often means direct descendants in the process tree world.
src/linux/proc.cpp
<https://reviews.apache.org/r/9794/#comment37214>
woah, didn't know this magic .second on set inserts.
- Vinod Kone
On March 7, 2013, 3:16 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9794/
> -----------------------------------------------------------
>
> (Updated March 7, 2013, 3:16 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> This is used in the following review for process based isolation resource usage.
>
>
> Diffs
> -----
>
> src/linux/proc.hpp e0825a4a9f9e2763e0c25d7319f220bfe7c7c29c
> src/linux/proc.cpp da30e9a860ba7a142015baf05feaa508bdcdfed5
> src/tests/proc_tests.cpp c3fa526dc2909ad7cb84a3a40c5850885b0065e0
>
> Diff: https://reviews.apache.org/r/9794/diff/
>
>
> Testing
> -------
>
> Added tests.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Added proc::children for linux.
Posted by Ben Mahler <be...@gmail.com>.
> On March 7, 2013, 7:58 a.m., Benjamin Hindman wrote:
> > src/linux/proc.cpp, line 61
> > <https://reviews.apache.org/r/9794/diff/1/?file=267809#file267809line61>
> >
> > Food for thought: I called it ProcessStatistics because it's reading /proc/[pid]/stat ... but after seeing more how it's being used it's not really "statistics" as much as "status". In fact, 'man 5 proc' even says "contains status information". Maybe we s/ProcessStatistics/ProcessStatus/?
Ditto for s/SystemStatistics/SystemStatus/
> On March 7, 2013, 7:58 a.m., Benjamin Hindman wrote:
> > src/linux/proc.cpp, line 76
> > <https://reviews.apache.org/r/9794/diff/1/?file=267809#file267809line76>
> >
> > Why not just pull the parent out first:
> > ---------------------------------
> > pid_t parent = parents.front();
> > parents.pop();
> >
> > // Search for children of parent.
> > ---------------------------------
> >
> > The algorithm is unnecessarily complicated by the implicitness of "current process" == parents.front() (and with no extra performance gain).
Much cleaner, thanks!
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9794/#review17521
-----------------------------------------------------------
On March 7, 2013, 3:16 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9794/
> -----------------------------------------------------------
>
> (Updated March 7, 2013, 3:16 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> This is used in the following review for process based isolation resource usage.
>
>
> Diffs
> -----
>
> src/linux/proc.hpp e0825a4a9f9e2763e0c25d7319f220bfe7c7c29c
> src/linux/proc.cpp da30e9a860ba7a142015baf05feaa508bdcdfed5
> src/tests/proc_tests.cpp c3fa526dc2909ad7cb84a3a40c5850885b0065e0
>
> Diff: https://reviews.apache.org/r/9794/diff/
>
>
> Testing
> -------
>
> Added tests.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Added proc::children for linux.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9794/#review17521
-----------------------------------------------------------
Ship it!
src/linux/proc.cpp
<https://reviews.apache.org/r/9794/#comment37187>
Food for thought: I called it ProcessStatistics because it's reading /proc/[pid]/stat ... but after seeing more how it's being used it's not really "statistics" as much as "status". In fact, 'man 5 proc' even says "contains status information". Maybe we s/ProcessStatistics/ProcessStatus/?
src/linux/proc.cpp
<https://reviews.apache.org/r/9794/#comment37186>
If you pondered s/_pid/pid/ I would not have argued.
src/linux/proc.cpp
<https://reviews.apache.org/r/9794/#comment37185>
You know, you can call this 'children' if you want. ;)
src/linux/proc.cpp
<https://reviews.apache.org/r/9794/#comment37188>
What's your reasoning to put this comment here instead of above the 'descendants' declaration above (or said another way, why declare the descendants alone)?
src/linux/proc.cpp
<https://reviews.apache.org/r/9794/#comment37184>
Why not just pull the parent out first:
---------------------------------
pid_t parent = parents.front();
parents.pop();
// Search for children of parent.
---------------------------------
The algorithm is unnecessarily complicated by the implicitness of "current process" == parents.front() (and with no extra performance gain).
- Benjamin Hindman
On March 7, 2013, 3:16 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9794/
> -----------------------------------------------------------
>
> (Updated March 7, 2013, 3:16 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> This is used in the following review for process based isolation resource usage.
>
>
> Diffs
> -----
>
> src/linux/proc.hpp e0825a4a9f9e2763e0c25d7319f220bfe7c7c29c
> src/linux/proc.cpp da30e9a860ba7a142015baf05feaa508bdcdfed5
> src/tests/proc_tests.cpp c3fa526dc2909ad7cb84a3a40c5850885b0065e0
>
> Diff: https://reviews.apache.org/r/9794/diff/
>
>
> Testing
> -------
>
> Added tests.
>
>
> Thanks,
>
> Ben Mahler
>
>