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:11:43 UTC
Review Request: Resource Monitoring 4: Added metering to statistics.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9092/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Vinod Kone.
Description
-------
This add metering into the statistics model. The metering was inspired by jie's monitoring reviews.
We'll need to compute metered data on top incoming raw data. The challenge is that some meters will require previous data:
For example:
1. cpu_time = 10 @ time 10;
2. cpu_time = 20 @ time 20; --> cpu_usage = 1.0 (100%) this is 20-10 / 20-10
3. cpu_time = 25 @ time 30; --> cpu_usage = 0.5 (50%) this is 25-20 / 30-20
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
Diff: https://reviews.apache.org/r/9092/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request: Resource Monitoring 4: Added metering to statistics.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9092/
-----------------------------------------------------------
(Updated Feb. 22, 2013, 12:31 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Benh review.
Rebased off trunk.
Description
-------
This add metering into the statistics model. The metering was inspired by jie's monitoring reviews.
We'll need to compute metered data on top incoming raw data. The challenge is that some meters will require previous data:
For example:
1. cpu_time = 10 @ time 10;
2. cpu_time = 20 @ time 20; --> cpu_usage = 1.0 (100%) this is 20-10 / 20-10
3. cpu_time = 25 @ time 30; --> cpu_usage = 0.5 (50%) this is 25-20 / 30-20
This addresses bug MESOS-324.
https://issues.apache.org/jira/browse/MESOS-324
Diffs (updated)
-----
third_party/libprocess/Makefile.am 468df4e1426b725cf1252300134bc13658eae090
third_party/libprocess/include/process/statistics.hpp faefad2e374e08e4e0f88750c4c4eee74bce62d7
third_party/libprocess/include/stout/owned.hpp PRE-CREATION
third_party/libprocess/src/statistics.cpp 29d08f05c6ab7b69d587a44aab860b4dfe645b89
third_party/libprocess/src/tests/statistics_tests.cpp 0aaab3526618171c7cfbd11d40d614344bcbfd0a
Diff: https://reviews.apache.org/r/9092/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request: Resource Monitoring 4: Added metering to statistics.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9092/#review16810
-----------------------------------------------------------
Ship it!
third_party/libprocess/include/process/statistics.hpp
<https://reviews.apache.org/r/9092/#comment35717>
Move '{'.
third_party/libprocess/include/process/statistics.hpp
<https://reviews.apache.org/r/9092/#comment35718>
Ditto.
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9092/#comment35719>
const
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9092/#comment35720>
Ditto.
- Benjamin Hindman
On Feb. 6, 2013, 11:14 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9092/
> -----------------------------------------------------------
>
> (Updated Feb. 6, 2013, 11:14 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> This add metering into the statistics model. The metering was inspired by jie's monitoring reviews.
>
> We'll need to compute metered data on top incoming raw data. The challenge is that some meters will require previous data:
> For example:
> 1. cpu_time = 10 @ time 10;
> 2. cpu_time = 20 @ time 20; --> cpu_usage = 1.0 (100%) this is 20-10 / 20-10
> 3. cpu_time = 25 @ time 30; --> cpu_usage = 0.5 (50%) this is 25-20 / 30-20
>
>
> This addresses bug MESOS-324.
> https://issues.apache.org/jira/browse/MESOS-324
>
>
> Diffs
> -----
>
> third_party/libprocess/Makefile.am dad1b65c3fdb7dbdad4e7c3d9c241cc4e89c3325
> third_party/libprocess/include/process/statistics.hpp 9e3041a6e2a8ef022eacacad00bc4d974a8e33c9
> third_party/libprocess/include/stout/owned.hpp PRE-CREATION
> third_party/libprocess/src/statistics.cpp 2fe8af83c6c63a0fa8cb2e9636f9289f0e3d7f2f
> third_party/libprocess/src/tests/statistics_tests.cpp 0aaab3526618171c7cfbd11d40d614344bcbfd0a
>
> Diff: https://reviews.apache.org/r/9092/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Resource Monitoring 4: Added metering to statistics.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9092/#review16811
-----------------------------------------------------------
third_party/libprocess/include/process/statistics.hpp
<https://reviews.apache.org/r/9092/#comment35721>
kill this
- Ben Mahler
On Feb. 6, 2013, 11:14 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9092/
> -----------------------------------------------------------
>
> (Updated Feb. 6, 2013, 11:14 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> This add metering into the statistics model. The metering was inspired by jie's monitoring reviews.
>
> We'll need to compute metered data on top incoming raw data. The challenge is that some meters will require previous data:
> For example:
> 1. cpu_time = 10 @ time 10;
> 2. cpu_time = 20 @ time 20; --> cpu_usage = 1.0 (100%) this is 20-10 / 20-10
> 3. cpu_time = 25 @ time 30; --> cpu_usage = 0.5 (50%) this is 25-20 / 30-20
>
>
> This addresses bug MESOS-324.
> https://issues.apache.org/jira/browse/MESOS-324
>
>
> Diffs
> -----
>
> third_party/libprocess/Makefile.am dad1b65c3fdb7dbdad4e7c3d9c241cc4e89c3325
> third_party/libprocess/include/process/statistics.hpp 9e3041a6e2a8ef022eacacad00bc4d974a8e33c9
> third_party/libprocess/include/stout/owned.hpp PRE-CREATION
> third_party/libprocess/src/statistics.cpp 2fe8af83c6c63a0fa8cb2e9636f9289f0e3d7f2f
> third_party/libprocess/src/tests/statistics_tests.cpp 0aaab3526618171c7cfbd11d40d614344bcbfd0a
>
> Diff: https://reviews.apache.org/r/9092/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Resource Monitoring 4: Added metering to statistics.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9092/
-----------------------------------------------------------
(Updated Feb. 6, 2013, 11:14 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
-benh review.
-Added stout/owned.hpp to abstract away unique ownership.
Description
-------
This add metering into the statistics model. The metering was inspired by jie's monitoring reviews.
We'll need to compute metered data on top incoming raw data. The challenge is that some meters will require previous data:
For example:
1. cpu_time = 10 @ time 10;
2. cpu_time = 20 @ time 20; --> cpu_usage = 1.0 (100%) this is 20-10 / 20-10
3. cpu_time = 25 @ time 30; --> cpu_usage = 0.5 (50%) this is 25-20 / 30-20
This addresses bug MESOS-324.
https://issues.apache.org/jira/browse/MESOS-324
Diffs (updated)
-----
third_party/libprocess/Makefile.am dad1b65c3fdb7dbdad4e7c3d9c241cc4e89c3325
third_party/libprocess/include/process/statistics.hpp 9e3041a6e2a8ef022eacacad00bc4d974a8e33c9
third_party/libprocess/include/stout/owned.hpp PRE-CREATION
third_party/libprocess/src/statistics.cpp 2fe8af83c6c63a0fa8cb2e9636f9289f0e3d7f2f
third_party/libprocess/src/tests/statistics_tests.cpp 0aaab3526618171c7cfbd11d40d614344bcbfd0a
Diff: https://reviews.apache.org/r/9092/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request: Resource Monitoring 4: Added metering to statistics.
Posted by Ben Mahler <be...@gmail.com>.
> On Feb. 5, 2013, 5:55 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/src/tests/statistics_tests.cpp, line 48
> > <https://reviews.apache.org/r/9092/diff/5/?file=254414#file254414line48>
> >
> > Are you concerned about floating point precision here?
Yes, this test was actually failing due to floating point imprecision.
It's possible the gtest comparison macro was losing the precision, but seeing as floating point literals are double by default, I'm not sure where the precision loss occurred.
> On Feb. 5, 2013, 5:55 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/src/tests/statistics_tests.cpp, line 78
> > <https://reviews.apache.org/r/9092/diff/5/?file=254414#file254414line78>
> >
> > Like here ... I personally don't mind the implicit conversion to double.
Ok, leaving the implicit conversions.
> On Feb. 5, 2013, 5:55 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/src/tests/statistics_tests.cpp, line 18
> > <https://reviews.apache.org/r/9092/diff/5/?file=254414#file254414line18>
> >
> > In retrospect, I thought I'd mention that you did 'Weeks(2)' in a previous review. I didn't mind it, but I wanted to mention it in case you were trying to be consistent and always pass a floating point number.
Thanks, going to change with Days(1).
> On Feb. 5, 2013, 5:55 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/src/statistics.cpp, lines 59-60
> > <https://reviews.apache.org/r/9092/diff/5/?file=254413#file254413line59>
> >
> > We haven' used the _ as a prefix for our members. I understand you can't call both the method and the field 'name', so you have three options: (1) put the underscore at the end of the field (the google style) (2) call the function or name something else or (3) make these fields be public. Since they're const we know they can't be modified and there is no real reason to have a "getter" (we have this style all over the code base, it keeps things pretty simple and is attractive because it enforces the immutability).
Great!
> On Feb. 5, 2013, 5:55 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/include/process/statistics.hpp, line 49
> > <https://reviews.apache.org/r/9092/diff/5/?file=254412#file254412line49>
> >
> > So, if I want to look this up is it a call to Statistics::get(context, meteredName)?
Yes, made this a little more evident in the comment.
> On Feb. 5, 2013, 5:55 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/include/process/statistics.hpp, line 48
> > <https://reviews.apache.org/r/9092/diff/5/?file=254412#file254412line48>
> >
> > What about taking a function that performs the computation? This seems more extensible in the long term (people can add their own meters and don't require Statistics to be involved). You can then pull the definition of time rate meter into a top level namespace (process::statistics::meters::TimeRateMeter).
I've changed this to pass in the Meter, which eliminates the meterType.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9092/#review16121
-----------------------------------------------------------
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/9092/
> -----------------------------------------------------------
>
> (Updated Feb. 1, 2013, 1:11 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> This add metering into the statistics model. The metering was inspired by jie's monitoring reviews.
>
> We'll need to compute metered data on top incoming raw data. The challenge is that some meters will require previous data:
> For example:
> 1. cpu_time = 10 @ time 10;
> 2. cpu_time = 20 @ time 20; --> cpu_usage = 1.0 (100%) this is 20-10 / 20-10
> 3. cpu_time = 25 @ time 30; --> cpu_usage = 0.5 (50%) this is 25-20 / 30-20
>
>
> 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/9092/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Resource Monitoring 4: Added metering to statistics.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9092/#review16121
-----------------------------------------------------------
third_party/libprocess/include/process/statistics.hpp
<https://reviews.apache.org/r/9092/#comment34446>
What about taking a function that performs the computation? This seems more extensible in the long term (people can add their own meters and don't require Statistics to be involved). You can then pull the definition of time rate meter into a top level namespace (process::statistics::meters::TimeRateMeter).
third_party/libprocess/include/process/statistics.hpp
<https://reviews.apache.org/r/9092/#comment34447>
So, if I want to look this up is it a call to Statistics::get(context, meteredName)?
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9092/#comment34449>
s/Seconds/const Seconds&/
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9092/#comment34448>
We haven' used the _ as a prefix for our members. I understand you can't call both the method and the field 'name', so you have three options: (1) put the underscore at the end of the field (the google style) (2) call the function or name something else or (3) make these fields be public. Since they're const we know they can't be modified and there is no real reason to have a "getter" (we have this style all over the code base, it keeps things pretty simple and is attractive because it enforces the immutability).
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9092/#comment34450>
This is too harsh. Need to return a Try.
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9092/#comment34451>
Again, too harsh. Also, why not return an error if someone attempts to add a duplicate meter? Doesn't that seem like a bug on their end?
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9092/#comment34452>
This will go away if the Meter is actually passed in. Note that we don't need to pass a Meter* since a meter should really be copyable (yes, it may have state, but I'm not sure that we want to support/encourage complicated meters that they can't be copied ).
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9092/#comment34454>
Temporary return optimization?
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9092/#comment34453>
This answers my question, but I think the comment above should be updated.
third_party/libprocess/src/tests/statistics_tests.cpp
<https://reviews.apache.org/r/9092/#comment34455>
In retrospect, I thought I'd mention that you did 'Weeks(2)' in a previous review. I didn't mind it, but I wanted to mention it in case you were trying to be consistent and always pass a floating point number.
third_party/libprocess/src/tests/statistics_tests.cpp
<https://reviews.apache.org/r/9092/#comment34457>
Are you concerned about floating point precision here?
third_party/libprocess/src/tests/statistics_tests.cpp
<https://reviews.apache.org/r/9092/#comment34458>
Like here ... I personally don't mind the implicit conversion to double.
- 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/9092/
> -----------------------------------------------------------
>
> (Updated Feb. 1, 2013, 1:11 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> This add metering into the statistics model. The metering was inspired by jie's monitoring reviews.
>
> We'll need to compute metered data on top incoming raw data. The challenge is that some meters will require previous data:
> For example:
> 1. cpu_time = 10 @ time 10;
> 2. cpu_time = 20 @ time 20; --> cpu_usage = 1.0 (100%) this is 20-10 / 20-10
> 3. cpu_time = 25 @ time 30; --> cpu_usage = 0.5 (50%) this is 25-20 / 30-20
>
>
> 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/9092/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Resource Monitoring 4: Added metering to statistics.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9092/
-----------------------------------------------------------
(Updated Feb. 1, 2013, 1:11 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Added tests.
Description
-------
This add metering into the statistics model. The metering was inspired by jie's monitoring reviews.
We'll need to compute metered data on top incoming raw data. The challenge is that some meters will require previous data:
For example:
1. cpu_time = 10 @ time 10;
2. cpu_time = 20 @ time 20; --> cpu_usage = 1.0 (100%) this is 20-10 / 20-10
3. cpu_time = 25 @ time 30; --> cpu_usage = 0.5 (50%) this is 25-20 / 30-20
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/9092/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request: Resource Monitoring 4: Added metering to statistics.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9092/
-----------------------------------------------------------
(Updated Jan. 31, 2013, 6:16 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
non-virtual meter methods
Description
-------
This add metering into the statistics model. The metering was inspired by jie's monitoring reviews.
We'll need to compute metered data on top incoming raw data. The challenge is that some meters will require previous data:
For example:
1. cpu_time = 10 @ time 10;
2. cpu_time = 20 @ time 20; --> cpu_usage = 1.0 (100%) this is 20-10 / 20-10
3. cpu_time = 25 @ time 30; --> cpu_usage = 0.5 (50%) this is 25-20 / 30-20
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
Diff: https://reviews.apache.org/r/9092/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request: Resource Monitoring 4: Added metering to statistics.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9092/
-----------------------------------------------------------
(Updated Jan. 31, 2013, 2:18 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Vinod's review.
Description
-------
This add metering into the statistics model. The metering was inspired by jie's monitoring reviews.
We'll need to compute metered data on top incoming raw data. The challenge is that some meters will require previous data:
For example:
1. cpu_time = 10 @ time 10;
2. cpu_time = 20 @ time 20; --> cpu_usage = 1.0 (100%) this is 20-10 / 20-10
3. cpu_time = 25 @ time 30; --> cpu_usage = 0.5 (50%) this is 25-20 / 30-20
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
Diff: https://reviews.apache.org/r/9092/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request: Resource Monitoring 4: Added metering to statistics.
Posted by Ben Mahler <be...@gmail.com>.
> On Jan. 30, 2013, 1:55 a.m., Vinod Kone wrote:
> >
They will both be writing to the same timeseries in that case.
I'll trust callers not to make that mistake, since they could make the same mistake across two semantically different non-metered statistics, and there's no way to stop them there either! On the positive side, it's completely non-fatal.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9092/#review15814
-----------------------------------------------------------
On Jan. 29, 2013, 12:10 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9092/
> -----------------------------------------------------------
>
> (Updated Jan. 29, 2013, 12:10 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> This add metering into the statistics model. The metering was inspired by jie's monitoring reviews.
>
> We'll need to compute metered data on top incoming raw data. The challenge is that some meters will require previous data:
> For example:
> 1. cpu_time = 10 @ time 10;
> 2. cpu_time = 20 @ time 20; --> cpu_usage = 1.0 (100%) this is 20-10 / 20-10
> 3. cpu_time = 25 @ time 30; --> cpu_usage = 0.5 (50%) this is 25-20 / 30-20
>
>
> 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
>
> Diff: https://reviews.apache.org/r/9092/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Resource Monitoring 4: Added metering to statistics.
Posted by Vinod Kone <vi...@gmail.com>.
> On Jan. 30, 2013, 1:55 a.m., Vinod Kone wrote:
> >
>
> Ben Mahler wrote:
> They will both be writing to the same timeseries in that case.
>
> I'll trust callers not to make that mistake, since they could make the same mistake across two semantically different non-metered statistics, and there's no way to stop them there either! On the positive side, it's completely non-fatal.
I disagree. We could/should never trust callers to not make mistakes. We should (as much as possible) program to disallow mistakes like that. Also, non-fatal doesn't mean its correct?
How about, you maintain a hashset of stat names (including meter names) and check against that?
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9092/#review15814
-----------------------------------------------------------
On Jan. 31, 2013, 6:16 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9092/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2013, 6:16 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> This add metering into the statistics model. The metering was inspired by jie's monitoring reviews.
>
> We'll need to compute metered data on top incoming raw data. The challenge is that some meters will require previous data:
> For example:
> 1. cpu_time = 10 @ time 10;
> 2. cpu_time = 20 @ time 20; --> cpu_usage = 1.0 (100%) this is 20-10 / 20-10
> 3. cpu_time = 25 @ time 30; --> cpu_usage = 0.5 (50%) this is 25-20 / 30-20
>
>
> 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
>
> Diff: https://reviews.apache.org/r/9092/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Resource Monitoring 4: Added metering to statistics.
Posted by Ben Mahler <be...@gmail.com>.
> On Jan. 30, 2013, 1:55 a.m., Vinod Kone wrote:
> >
>
> Ben Mahler wrote:
> They will both be writing to the same timeseries in that case.
>
> I'll trust callers not to make that mistake, since they could make the same mistake across two semantically different non-metered statistics, and there's no way to stop them there either! On the positive side, it's completely non-fatal.
>
> Vinod Kone wrote:
> I disagree. We could/should never trust callers to not make mistakes. We should (as much as possible) program to disallow mistakes like that. Also, non-fatal doesn't mean its correct?
>
> How about, you maintain a hashset of stat names (including meter names) and check against that?
I agree with your disagreement ;) I think in this case, we're providing a primitive to write statistics, callers making mistakes is equivalent to multiple people writing to the same file, which is bad, but the kernel cannot prevent this, nor would it want to!
Maybe this helps, to see why your suggestion will not work:
Across stats:
set(context, name, value); // now the hashset contains <context, name>
set(context, name, value); // hey! <context, name> is in our set! Is this ok? Depends on the caller's intent!
Across meters:
meter(context, name, meter_name); // now the hashset contains <context, meter_name> after a value comes in
meter(context, name, meter_name); // hey! <context, meter_name> is in our set! Is this ok? Depends on the caller's intent!
Across stats/meters:
set(context, meter_name, value); // now hashset contains <context, meter_name>
meter(context, name, meter_name); // hey! <context, meter_name> exists! Is this ok? In this case, no, but how do we know?
This last case is the only enforceable check, but a hashset is not sufficient. We need to track the "source" (i.e. raw stat? metered stat?) to do this check.
I think this starts to store a lot of meta-data about the stats, which gets clunky and inefficient.
I'd like to think of Statistics as a simple, map-like primitive.
Did this make sense, or did I misunderstand?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9092/#review15814
-----------------------------------------------------------
On Jan. 31, 2013, 6:16 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9092/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2013, 6:16 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> This add metering into the statistics model. The metering was inspired by jie's monitoring reviews.
>
> We'll need to compute metered data on top incoming raw data. The challenge is that some meters will require previous data:
> For example:
> 1. cpu_time = 10 @ time 10;
> 2. cpu_time = 20 @ time 20; --> cpu_usage = 1.0 (100%) this is 20-10 / 20-10
> 3. cpu_time = 25 @ time 30; --> cpu_usage = 0.5 (50%) this is 25-20 / 30-20
>
>
> 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
>
> Diff: https://reviews.apache.org/r/9092/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Resource Monitoring 4: Added metering to statistics.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9092/#review15814
-----------------------------------------------------------
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9092/#comment34065>
s/;/ << "Meter name cannot be same as stat name";
Alˆso, what happens if the meter name conflicts with a different stat in the same context?
- Vinod Kone
On Jan. 29, 2013, 12:10 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9092/
> -----------------------------------------------------------
>
> (Updated Jan. 29, 2013, 12:10 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> This add metering into the statistics model. The metering was inspired by jie's monitoring reviews.
>
> We'll need to compute metered data on top incoming raw data. The challenge is that some meters will require previous data:
> For example:
> 1. cpu_time = 10 @ time 10;
> 2. cpu_time = 20 @ time 20; --> cpu_usage = 1.0 (100%) this is 20-10 / 20-10
> 3. cpu_time = 25 @ time 30; --> cpu_usage = 0.5 (50%) this is 25-20 / 30-20
>
>
> 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
>
> Diff: https://reviews.apache.org/r/9092/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Resource Monitoring 4: Added metering to statistics.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9092/
-----------------------------------------------------------
(Updated Jan. 29, 2013, 12:10 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Vinod's review.
Description
-------
This add metering into the statistics model. The metering was inspired by jie's monitoring reviews.
We'll need to compute metered data on top incoming raw data. The challenge is that some meters will require previous data:
For example:
1. cpu_time = 10 @ time 10;
2. cpu_time = 20 @ time 20; --> cpu_usage = 1.0 (100%) this is 20-10 / 20-10
3. cpu_time = 25 @ time 30; --> cpu_usage = 0.5 (50%) this is 25-20 / 30-20
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
Diff: https://reviews.apache.org/r/9092/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request: Resource Monitoring 4: Added metering to statistics.
Posted by Ben Mahler <be...@gmail.com>.
> On Jan. 28, 2013, 8:30 p.m., Vinod Kone wrote:
> > third_party/libprocess/src/statistics.cpp, line 45
> > <https://reviews.apache.org/r/9092/diff/1/?file=251533#file251533line45>
> >
> > Put the initializer list on its own line.
I'd prefer to keep it on one line if it's a trivial initializer list.
I've seen this done in many places:
stout/json.hpp
stout/try.hpp
stout/option.hpp
stout/result.hpp
stout/duration.hpp
etc
> On Jan. 28, 2013, 8:30 p.m., Vinod Kone wrote:
> > third_party/libprocess/src/statistics.cpp, line 184
> > <https://reviews.apache.org/r/9092/diff/1/?file=251533#file251533line184>
> >
> > was this necessary to suppress gcc warning?
I added it for consistency. I prefer breaks or returns in switch cases that aren't expected to fall through. (Even though this one has a LOG(FATAL)).
> On Jan. 28, 2013, 8:30 p.m., Vinod Kone wrote:
> > third_party/libprocess/src/statistics.cpp, line 67
> > <https://reviews.apache.org/r/9092/diff/1/?file=251533#file251533line67>
> >
> > Why virtual?
See my comment about virtual destructors from the previous review.
> On Jan. 28, 2013, 8:30 p.m., Vinod Kone wrote:
> > third_party/libprocess/src/statistics.cpp, line 167
> > <https://reviews.apache.org/r/9092/diff/1/?file=251533#file251533line167>
> >
> > s/for existing meter/for an existing meter that matches the meteredName and meteredType/
> On Jan. 28, 2013, 8:30 p.m., Vinod Kone wrote:
> > third_party/libprocess/src/statistics.cpp, line 171
> > <https://reviews.apache.org/r/9092/diff/1/?file=251533#file251533line171>
> >
> > What if meterType doesn't match?
> >
> > As I mentioned above, meter->value() would be useful to have.
For checking type, I think the only useful thing would be to ensure the type matches when a duplicate is found via a CHECK. This prevents the mistake of:
meter (context, name, TYPE1, meterName)
meter (context, name, TYPE2, meterName) // Was a no-op, but I now CHECK for this.. since it's a mistake by the caller.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9092/#review15764
-----------------------------------------------------------
On Jan. 24, 2013, 9:11 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9092/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2013, 9:11 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> This add metering into the statistics model. The metering was inspired by jie's monitoring reviews.
>
> We'll need to compute metered data on top incoming raw data. The challenge is that some meters will require previous data:
> For example:
> 1. cpu_time = 10 @ time 10;
> 2. cpu_time = 20 @ time 20; --> cpu_usage = 1.0 (100%) this is 20-10 / 20-10
> 3. cpu_time = 25 @ time 30; --> cpu_usage = 0.5 (50%) this is 25-20 / 30-20
>
>
> 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
>
> Diff: https://reviews.apache.org/r/9092/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Resource Monitoring 4: Added metering to statistics.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9092/#review15764
-----------------------------------------------------------
third_party/libprocess/include/process/statistics.hpp
<https://reviews.apache.org/r/9092/#comment33928>
s/, they/. They/
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9092/#comment33929>
Put the initializer list on its own line.
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9092/#comment33932>
This should probably also hold its type?
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9092/#comment33930>
Initializer on a different line.
Also, I didn't realize (expect) Seconds to take a -ve value!?
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9092/#comment33931>
Why virtual?
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9092/#comment33934>
Its useful to explain what the mapping is.
Same, for the above map.
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9092/#comment33936>
s/for existing meter/for an existing meter that matches the meteredName and meteredType/
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9092/#comment33933>
What if meterType doesn't match?
As I mentioned above, meter->value() would be useful to have.
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9092/#comment33937>
was this necessary to suppress gcc warning?
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9092/#comment33935>
Is there any way you can ensure that meteredName doesn't conflict with a stat name?
- Vinod Kone
On Jan. 24, 2013, 9:11 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9092/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2013, 9:11 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> This add metering into the statistics model. The metering was inspired by jie's monitoring reviews.
>
> We'll need to compute metered data on top incoming raw data. The challenge is that some meters will require previous data:
> For example:
> 1. cpu_time = 10 @ time 10;
> 2. cpu_time = 20 @ time 20; --> cpu_usage = 1.0 (100%) this is 20-10 / 20-10
> 3. cpu_time = 25 @ time 30; --> cpu_usage = 0.5 (50%) this is 25-20 / 30-20
>
>
> 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
>
> Diff: https://reviews.apache.org/r/9092/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>