> 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 > >
