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/01/24 10:06:26 UTC
Review Request: Resource Monitoring 3: Added context to statistics.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9091/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Vinod Kone.
Description
-------
This adds context into the Statistics model. Contexts provide a clean way to logically group stats (e.g. contexts: "master", "slave", "monitor", "libprocess", etc).
This addresses bug MESOS-324.
https://issues.apache.org/jira/browse/MESOS-324
Diffs
-----
third_party/libprocess/include/process/statistics.hpp 9e3041a6e2a8ef022eacacad00bc4d974a8e33c9
third_party/libprocess/src/statistics.cpp 2fe8af83c6c63a0fa8cb2e9636f9289f0e3d7f2f
third_party/libprocess/src/statistics_tests.cpp 0aaab3526618171c7cfbd11d40d614344bcbfd0a
Diff: https://reviews.apache.org/r/9091/diff/
Testing
-------
make check + libprocess make check
Thanks,
Ben Mahler
Re: Review Request: Resource Monitoring 3: Added context to statistics.
Posted by Vinod Kone <vi...@gmail.com>.
> On Jan. 28, 2013, 8:08 p.m., Vinod Kone wrote:
> > third_party/libprocess/src/statistics.cpp, line 50
> > <https://reviews.apache.org/r/9091/diff/1/?file=251530#file251530line50>
> >
> > Why virtual? Are you expecting this to be subclassed?
>
> Ben Mahler wrote:
> virtual so that deletion of a Process pointer pointing to a StatisticsProcess will call this destructor. I think destructors should always be virtual when dealing with inheritance: http://www.learncpp.com/cpp-tutorial/123-virtual-destructors-virtual-assignment-and-overriding-virtualization/
Yes. I was just wondering if you ever anticipate passing StatiscticsProcess* as Process*. I guess virtual is always safe.
> On Jan. 28, 2013, 8:08 p.m., Vinod Kone wrote:
> > third_party/libprocess/src/statistics.cpp, line 176
> > <https://reviews.apache.org/r/9091/diff/1/?file=251530#file251530line176>
> >
> > so is it ok to mutate the list and use its iterator at the same time!?
>
> Ben Mahler wrote:
> This was the original code from Statistics, but reading up on the map erase:
> "Iterators, pointers and references referring to elements removed by the function are invalidated.
> All other iterators, pointers and references keep their validity."
>
> So technically, we would need to advance to the next element to avoid invalidation. (see: http://stackoverflow.com/a/5673209)
>
> But I've included a simpler fix here.
clever :)
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9091/#review15763
-----------------------------------------------------------
On Jan. 28, 2013, 11:32 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9091/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2013, 11:32 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> This adds context into the Statistics model. Contexts provide a clean way to logically group stats (e.g. contexts: "master", "slave", "monitor", "libprocess", etc).
>
>
> This addresses bug MESOS-324.
> https://issues.apache.org/jira/browse/MESOS-324
>
>
> Diffs
> -----
>
> third_party/libprocess/include/process/statistics.hpp 9e3041a6e2a8ef022eacacad00bc4d974a8e33c9
> third_party/libprocess/src/statistics.cpp 2fe8af83c6c63a0fa8cb2e9636f9289f0e3d7f2f
> third_party/libprocess/src/statistics_tests.cpp 0aaab3526618171c7cfbd11d40d614344bcbfd0a
>
> Diff: https://reviews.apache.org/r/9091/diff/
>
>
> Testing
> -------
>
> make check + libprocess make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Resource Monitoring 3: Added context to statistics.
Posted by Ben Mahler <be...@gmail.com>.
> On Jan. 28, 2013, 8:08 p.m., Vinod Kone wrote:
> > third_party/libprocess/src/statistics.cpp, line 50
> > <https://reviews.apache.org/r/9091/diff/1/?file=251530#file251530line50>
> >
> > Why virtual? Are you expecting this to be subclassed?
virtual so that deletion of a Process pointer pointing to a StatisticsProcess will call this destructor. I think destructors should always be virtual when dealing with inheritance: http://www.learncpp.com/cpp-tutorial/123-virtual-destructors-virtual-assignment-and-overriding-virtualization/
> On Jan. 28, 2013, 8:08 p.m., Vinod Kone wrote:
> > third_party/libprocess/src/statistics.cpp, line 139
> > <https://reviews.apache.org/r/9091/diff/1/?file=251530#file251530line139>
> >
> > What if context doesn't exist?
The [] operator will insert it if missing, which is the desired behavior here, am I missing anything else?
> On Jan. 28, 2013, 8:08 p.m., Vinod Kone wrote:
> > third_party/libprocess/src/statistics.cpp, line 176
> > <https://reviews.apache.org/r/9091/diff/1/?file=251530#file251530line176>
> >
> > so is it ok to mutate the list and use its iterator at the same time!?
This was the original code from Statistics, but reading up on the map erase:
"Iterators, pointers and references referring to elements removed by the function are invalidated.
All other iterators, pointers and references keep their validity."
So technically, we would need to advance to the next element to avoid invalidation. (see: http://stackoverflow.com/a/5673209)
But I've included a simpler fix here.
> On Jan. 28, 2013, 8:08 p.m., Vinod Kone wrote:
> > third_party/libprocess/src/statistics.cpp, line 173
> > <https://reviews.apache.org/r/9091/diff/1/?file=251530#file251530line173>
> >
> > kinda weird to the name of the iterator that traverses the whole map as 'start'?
> >
> > s/start/it?
Well this iterator is always referring to the start, since we continually truncate off the first element.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9091/#review15763
-----------------------------------------------------------
On Jan. 24, 2013, 9:06 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9091/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2013, 9:06 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> This adds context into the Statistics model. Contexts provide a clean way to logically group stats (e.g. contexts: "master", "slave", "monitor", "libprocess", etc).
>
>
> This addresses bug MESOS-324.
> https://issues.apache.org/jira/browse/MESOS-324
>
>
> Diffs
> -----
>
> third_party/libprocess/include/process/statistics.hpp 9e3041a6e2a8ef022eacacad00bc4d974a8e33c9
> third_party/libprocess/src/statistics.cpp 2fe8af83c6c63a0fa8cb2e9636f9289f0e3d7f2f
> third_party/libprocess/src/statistics_tests.cpp 0aaab3526618171c7cfbd11d40d614344bcbfd0a
>
> Diff: https://reviews.apache.org/r/9091/diff/
>
>
> Testing
> -------
>
> make check + libprocess make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Resource Monitoring 3: Added context to statistics.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9091/#review15763
-----------------------------------------------------------
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9091/#comment33923>
Why virtual? Are you expecting this to be subclassed?
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9091/#comment33924>
What if context doesn't exist?
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9091/#comment33925>
ditto
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9091/#comment33926>
kinda weird to the name of the iterator that traverses the whole map as 'start'?
s/start/it?
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9091/#comment33927>
so is it ok to mutate the list and use its iterator at the same time!?
- Vinod Kone
On Jan. 24, 2013, 9:06 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9091/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2013, 9:06 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> This adds context into the Statistics model. Contexts provide a clean way to logically group stats (e.g. contexts: "master", "slave", "monitor", "libprocess", etc).
>
>
> This addresses bug MESOS-324.
> https://issues.apache.org/jira/browse/MESOS-324
>
>
> Diffs
> -----
>
> third_party/libprocess/include/process/statistics.hpp 9e3041a6e2a8ef022eacacad00bc4d974a8e33c9
> third_party/libprocess/src/statistics.cpp 2fe8af83c6c63a0fa8cb2e9636f9289f0e3d7f2f
> third_party/libprocess/src/statistics_tests.cpp 0aaab3526618171c7cfbd11d40d614344bcbfd0a
>
> Diff: https://reviews.apache.org/r/9091/diff/
>
>
> Testing
> -------
>
> make check + libprocess make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Resource Monitoring 3: Added context to statistics.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9091/#review15813
-----------------------------------------------------------
Ship it!
- Vinod Kone
On Jan. 28, 2013, 11:32 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9091/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2013, 11:32 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> This adds context into the Statistics model. Contexts provide a clean way to logically group stats (e.g. contexts: "master", "slave", "monitor", "libprocess", etc).
>
>
> This addresses bug MESOS-324.
> https://issues.apache.org/jira/browse/MESOS-324
>
>
> Diffs
> -----
>
> third_party/libprocess/include/process/statistics.hpp 9e3041a6e2a8ef022eacacad00bc4d974a8e33c9
> third_party/libprocess/src/statistics.cpp 2fe8af83c6c63a0fa8cb2e9636f9289f0e3d7f2f
> third_party/libprocess/src/statistics_tests.cpp 0aaab3526618171c7cfbd11d40d614344bcbfd0a
>
> Diff: https://reviews.apache.org/r/9091/diff/
>
>
> Testing
> -------
>
> make check + libprocess make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Resource Monitoring 3: Added context to statistics.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9091/#review16809
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Hindman
On Feb. 6, 2013, 2:30 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9091/
> -----------------------------------------------------------
>
> (Updated Feb. 6, 2013, 2:30 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> This adds context into the Statistics model. Contexts provide a clean way to logically group stats (e.g. contexts: "master", "slave", "monitor", "libprocess", etc).
>
>
> This addresses bug MESOS-324.
> https://issues.apache.org/jira/browse/MESOS-324
>
>
> Diffs
> -----
>
> third_party/libprocess/include/process/statistics.hpp 9e3041a6e2a8ef022eacacad00bc4d974a8e33c9
> third_party/libprocess/src/statistics.cpp 2fe8af83c6c63a0fa8cb2e9636f9289f0e3d7f2f
> third_party/libprocess/src/tests/statistics_tests.cpp 0aaab3526618171c7cfbd11d40d614344bcbfd0a
>
> Diff: https://reviews.apache.org/r/9091/diff/
>
>
> Testing
> -------
>
> make check + libprocess make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Resource Monitoring 3: Added context to statistics.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9091/
-----------------------------------------------------------
(Updated Feb. 22, 2013, 12:31 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Rebased off trunk.
Description
-------
This adds context into the Statistics model. Contexts provide a clean way to logically group stats (e.g. contexts: "master", "slave", "monitor", "libprocess", etc).
This addresses bug MESOS-324.
https://issues.apache.org/jira/browse/MESOS-324
Diffs (updated)
-----
third_party/libprocess/include/process/statistics.hpp faefad2e374e08e4e0f88750c4c4eee74bce62d7
third_party/libprocess/src/statistics.cpp 29d08f05c6ab7b69d587a44aab860b4dfe645b89
third_party/libprocess/src/tests/statistics_tests.cpp 0aaab3526618171c7cfbd11d40d614344bcbfd0a
Diff: https://reviews.apache.org/r/9091/diff/
Testing
-------
make check + libprocess make check
Thanks,
Ben Mahler
Re: Review Request: Resource Monitoring 3: Added context to statistics.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9091/
-----------------------------------------------------------
(Updated Feb. 6, 2013, 2:30 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
benh review
Description
-------
This adds context into the Statistics model. Contexts provide a clean way to logically group stats (e.g. contexts: "master", "slave", "monitor", "libprocess", etc).
This addresses bug MESOS-324.
https://issues.apache.org/jira/browse/MESOS-324
Diffs (updated)
-----
third_party/libprocess/include/process/statistics.hpp 9e3041a6e2a8ef022eacacad00bc4d974a8e33c9
third_party/libprocess/src/statistics.cpp 2fe8af83c6c63a0fa8cb2e9636f9289f0e3d7f2f
third_party/libprocess/src/tests/statistics_tests.cpp 0aaab3526618171c7cfbd11d40d614344bcbfd0a
Diff: https://reviews.apache.org/r/9091/diff/
Testing
-------
make check + libprocess make check
Thanks,
Ben Mahler
Re: Review Request: Resource Monitoring 3: Added context to statistics.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9091/#review16068
-----------------------------------------------------------
third_party/libprocess/include/process/statistics.hpp
<https://reviews.apache.org/r/9091/#comment34373>
This comment is now out of date.
third_party/libprocess/include/process/statistics.hpp
<https://reviews.apache.org/r/9091/#comment34374>
Update the comment with "... at the current clock time or at some specified time."
third_party/libprocess/include/process/statistics.hpp
<https://reviews.apache.org/r/9091/#comment34375>
Need to bring in 'process/clock.hpp' now. Also, s/Seconds time/const Seconds& time/.
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9091/#comment34384>
s/Seconds/const Seconds&/
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9091/#comment34379>
NICE cleanup! I like the macroness of it!
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9091/#comment34381>
You can 'else if'.
- Benjamin Hindman
On Feb. 1, 2013, 1:11 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9091/
> -----------------------------------------------------------
>
> (Updated Feb. 1, 2013, 1:11 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> This adds context into the Statistics model. Contexts provide a clean way to logically group stats (e.g. contexts: "master", "slave", "monitor", "libprocess", etc).
>
>
> This addresses bug MESOS-324.
> https://issues.apache.org/jira/browse/MESOS-324
>
>
> Diffs
> -----
>
> third_party/libprocess/include/process/statistics.hpp 9e3041a6e2a8ef022eacacad00bc4d974a8e33c9
> third_party/libprocess/src/statistics.cpp 2fe8af83c6c63a0fa8cb2e9636f9289f0e3d7f2f
> third_party/libprocess/src/tests/statistics_tests.cpp 0aaab3526618171c7cfbd11d40d614344bcbfd0a
>
> Diff: https://reviews.apache.org/r/9091/diff/
>
>
> Testing
> -------
>
> make check + libprocess make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Resource Monitoring 3: Added context to statistics.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9091/
-----------------------------------------------------------
(Updated Feb. 1, 2013, 1:11 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Test updates.
Description
-------
This adds context into the Statistics model. Contexts provide a clean way to logically group stats (e.g. contexts: "master", "slave", "monitor", "libprocess", etc).
This addresses bug MESOS-324.
https://issues.apache.org/jira/browse/MESOS-324
Diffs (updated)
-----
third_party/libprocess/include/process/statistics.hpp 9e3041a6e2a8ef022eacacad00bc4d974a8e33c9
third_party/libprocess/src/statistics.cpp 2fe8af83c6c63a0fa8cb2e9636f9289f0e3d7f2f
third_party/libprocess/src/tests/statistics_tests.cpp 0aaab3526618171c7cfbd11d40d614344bcbfd0a
Diff: https://reviews.apache.org/r/9091/diff/
Testing
-------
make check + libprocess make check
Thanks,
Ben Mahler
Re: Review Request: Resource Monitoring 3: Added context to statistics.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9091/
-----------------------------------------------------------
(Updated Jan. 28, 2013, 11:32 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Vinod's review.
Description
-------
This adds context into the Statistics model. Contexts provide a clean way to logically group stats (e.g. contexts: "master", "slave", "monitor", "libprocess", etc).
This addresses bug MESOS-324.
https://issues.apache.org/jira/browse/MESOS-324
Diffs (updated)
-----
third_party/libprocess/include/process/statistics.hpp 9e3041a6e2a8ef022eacacad00bc4d974a8e33c9
third_party/libprocess/src/statistics.cpp 2fe8af83c6c63a0fa8cb2e9636f9289f0e3d7f2f
third_party/libprocess/src/statistics_tests.cpp 0aaab3526618171c7cfbd11d40d614344bcbfd0a
Diff: https://reviews.apache.org/r/9091/diff/
Testing
-------
make check + libprocess make check
Thanks,
Ben Mahler