> On Feb. 5, 2013, 6:26 p.m., Benjamin Hindman wrote: > > 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.
I held off on a struct but after thinking about it, I think adding a TimeSeries struct would be nice! Done, and it's much cleaner. I can also expand on this struct later to isolate the TimeSeries functionality: -Truncation logic: this will get more complex when adding coarseness. -Set, Get, etc if needed. > On Feb. 5, 2013, 6:26 p.m., Benjamin Hindman wrote: > > third_party/libprocess/include/process/clock.hpp, line 9 > > <https://reviews.apache.org/r/9093/diff/5/?file=254415#file254415line9> > > > > 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. ;) Feel free to email me with your partial patch. I'm going to punt for resource monitoring, I may get to after in an independent change. > On Feb. 5, 2013, 6:26 p.m., Benjamin Hindman wrote: > > third_party/libprocess/include/process/statistics.hpp, line 19 > > <https://reviews.apache.org/r/9093/diff/5/?file=254416#file254416line19> > > > > Eventually we'll want to make these "configurable" via environment > > variables (that's been our standard for libprocess). I only see: $ grep -R getenv third_party/libprocess/src third_party/libprocess/src/process.cpp: value = getenv("LIBPROCESS_IP"); third_party/libprocess/src/process.cpp: value = getenv("LIBPROCESS_PORT"); I can add this now if you like: "LIBPROCESS_STATISTICS_TRUNCATION_INTERVAL", "LIBPROCESS_STATS_TRUNCATION"? > On Feb. 5, 2013, 6:26 p.m., Benjamin Hindman wrote: > > third_party/libprocess/include/process/statistics.hpp, line 25 > > <https://reviews.apache.org/r/9093/diff/5/?file=254416#file254416line25> > > > > 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. Right, I'm on the same page, I was focused on providing determinism on the upper bound to guarantee that this won't blow up in memory (the tunable you mentioned), and possibly being intelligent. I'll rewrite this comment to describe the ideal design. > On Feb. 5, 2013, 6:26 p.m., Benjamin Hindman wrote: > > third_party/libprocess/include/process/statistics.hpp, line 67 > > <https://reviews.apache.org/r/9093/diff/5/?file=254416#file254416line67> > > > > As in, it will be part of the snapshot until the window expires? Nope: // 1. The statistic will no longer be part of the snapshot. But it will remain part of the timeseries.json. > On Feb. 5, 2013, 6:26 p.m., Benjamin Hindman wrote: > > third_party/libprocess/src/statistics.cpp, line 120 > > <https://reviews.apache.org/r/9093/diff/5/?file=254417#file254417line120> > > > > 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!). I don't see contexts being used ephemerally, but I could be convinced otherwise with a good example. This is why I did not include such a method. > On Feb. 5, 2013, 6:26 p.m., Benjamin Hindman wrote: > > third_party/libprocess/src/statistics.cpp, line 132 > > <https://reviews.apache.org/r/9093/diff/5/?file=254417#file254417line132> > > > > 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. Good point! - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9093/#review16124 ----------------------------------------------------------- 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 > >
