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