----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9093/#review16124 -----------------------------------------------------------
My high level thought for this review: What about adding a Statistic struct that contains two fields, (1) the 'map<Seconds, double> values' and a 'bool archived'. The one thing I like about that is that we don't need to keep logic around for deciding when to clean up the archived map. third_party/libprocess/include/process/clock.hpp <https://reviews.apache.org/r/9093/#comment34459> When we changed most 'double' values to use Seconds or Duration I did this here, but there was some mess and never got that part fully committed. I still have an old branch for it though, so ping me if you take this on and want to do less work. ;) third_party/libprocess/include/process/statistics.hpp <https://reviews.apache.org/r/9093/#comment34466> Eventually we'll want to make these "configurable" via environment variables (that's been our standard for libprocess). third_party/libprocess/include/process/statistics.hpp <https://reviews.apache.org/r/9093/#comment34460> As the bottom of your comment alludes, the original intent for window was to coarsen the granularity of data over time. That way, for high frequency statistics, there are a lot of _recent_ data points (fine granularity), and less _older_ data points (coarse granularity). I could see adding a tunable which decides how many total data points to keep around which informs how often to delete old data points while still keeping a window worth of information. This is much more valuable than just keeping N data points around because N data points might not give you enough history (over some window of time) to understand the general shape of the data. third_party/libprocess/include/process/statistics.hpp <https://reviews.apache.org/r/9093/#comment34463> s/timeseries/time series/ third_party/libprocess/include/process/statistics.hpp <https://reviews.apache.org/r/9093/#comment34464> As in, it will be part of the snapshot until the window expires? third_party/libprocess/include/process/statistics.hpp <https://reviews.apache.org/r/9093/#comment34462> This line seems redundant. third_party/libprocess/src/statistics.cpp <https://reviews.apache.org/r/9093/#comment34467> Would one ever want to archive an entire context? Seems like a helper that loops through all statistics for a name and calls this version of the function might be very useful (and avoid people having to remember all fo the statistics they might have created!). third_party/libprocess/src/statistics.cpp <https://reviews.apache.org/r/9093/#comment34468> First, I didn't understand this comment and I think making it more explicit would be helpful, i.e.,: "We wait 'window' time until trying to do our first truncation across all statistics because no values will get truncated until after window passes". Second, I don't agree with this since someone could set values outside the window. third_party/libprocess/src/statistics.cpp <https://reviews.apache.org/r/9093/#comment34465> s/timeseries/time series/g Also, let's lean towards sentences that leave less ambiguity or require thought: 'Returns true if the time series is empty." - Benjamin Hindman On Feb. 1, 2013, 1:12 a.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9093/ > ----------------------------------------------------------- > > (Updated Feb. 1, 2013, 1:12 a.m.) > > > Review request for mesos, Benjamin Hindman and Vinod Kone. > > > Description > ------- > > Some statistics will be ephemeral. (e.g. executor resource usage information). > It does not make sense to keep a value of these stats in the snapshot. > > > This addresses bug MESOS-324. > https://issues.apache.org/jira/browse/MESOS-324 > > > Diffs > ----- > > third_party/libprocess/include/process/clock.hpp > 1a98c6a8b50fdad5308321413121f86bea135d37 > 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/9093/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Ben Mahler > >
