> On Jan. 15, 2014, 1:31 a.m., Benjamin Hindman wrote: > > 3rdparty/libprocess/include/process/statistics.hpp, line 5 > > <https://reviews.apache.org/r/13603/diff/2/?file=422661#file422661line5> > > > > Is this for std::pair? Maybe a comment?
Removed this include since I no longer need it. > On Jan. 15, 2014, 1:31 a.m., Benjamin Hindman wrote: > > 3rdparty/libprocess/src/statistics.cpp, line 229 > > <https://reviews.apache.org/r/13603/diff/2/?file=422662#file422662line229> > > > > Why not statistics[context][name] = ? Ah, an artifact of when I had TimeSeries as a const struct, which prevented assignability. > On Jan. 15, 2014, 1:31 a.m., Benjamin Hindman wrote: > > 3rdparty/libprocess/include/process/statistics.hpp, lines 126-127 > > <https://reviews.apache.org/r/13603/diff/2/?file=422661#file422661line126> > > > > What about just doing 'capacity(std::max(3, _capacity))' above instead? > > I don't think keeping around another 2 instances of T will likely be that > > big of a deal. Sounds good. > On Jan. 15, 2014, 1:31 a.m., Benjamin Hindman wrote: > > 3rdparty/libprocess/include/process/statistics.hpp, line 135 > > <https://reviews.apache.org/r/13603/diff/2/?file=422661#file422661line135> > > > > s/lg/log/ Ah, I thought lg was standard for log base 2, but I guess not: http://en.wikipedia.org/wiki/Logarithm#Particular_bases > On Jan. 15, 2014, 1:31 a.m., Benjamin Hindman wrote: > > 3rdparty/libprocess/src/statistics.cpp, lines 45-47 > > <https://reviews.apache.org/r/13603/diff/2/?file=422662#file422662line45> > > > > Formatting? Reverted my alignment, I think we should allow some table-like formatting when it makes things easier to read, but this isn't a great example. > On Jan. 15, 2014, 1:31 a.m., Benjamin Hindman wrote: > > 3rdparty/libprocess/include/process/statistics.hpp, line 165 > > <https://reviews.apache.org/r/13603/diff/2/?file=422661#file422661line165> > > > > What about a TimeSeries::Value struct and returning that instead of a > > pair in order to capture the semantics explicitly? I like that, I've also updated to return a vector<Value> as opposed to a map<Time, T> which was exposing an implantation detail. > On Jan. 15, 2014, 1:31 a.m., Benjamin Hindman wrote: > > 3rdparty/libprocess/include/process/statistics.hpp, line 214 > > <https://reviews.apache.org/r/13603/diff/2/?file=422661#file422661line214> > > > > It would be great to have some more comments and maybe even some CHECKs > > that capture your expected semantics re: 'next'. For example, the first > > time this is called you expect 'index' to be none and that's when you set > > 'next'. Likewise, after a truncation it might be the case that 'next' is > > invalid and you need to reset it. A comment down by 'Option<size_t> index;' > > would be good too (saying something like it starts none until the first > > sparsify call). I've added a comment on for 'index' and 'next' and I cleaned up sparsify() further, there's now only one place where we deal with resetting 'index'. > On Jan. 15, 2014, 1:31 a.m., Benjamin Hindman wrote: > > 3rdparty/libprocess/src/tests/statistics_tests.cpp, line 73 > > <https://reviews.apache.org/r/13603/diff/2/?file=422663#file422663line73> > > > > By a "round of truncation", do you mean sparsification? > > > > IIUC, I would expect that you can add less things in order for > > sparsification to occur. Perhaps you're being more general here? But to > > capture the algorithm I think that you could add strictly less. > > > > Also, given that some truncation can occur after some time, should you > > pause the clock for this test? I've cleaned up the tests significantly, let me know if you find them more clear: 1. TimeSeries tests are now in a separate file. 2. The sparsification tests and truncation tests now use a List construction to compare values more explicitly. - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13603/#review31835 ----------------------------------------------------------- On Jan. 14, 2014, 7:08 a.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/13603/ > ----------------------------------------------------------- > > (Updated Jan. 14, 2014, 7:08 a.m.) > > > Review request for mesos, Benjamin Hindman and Jie Yu. > > > Repository: mesos-git > > > Description > ------- > > The first part of this change is the removal of the archive operation. > > This also exposes a TimeSeries data structure used by Statistics. > > The goal here is to provide reasonable historical information for high > frequency time series that rapidly exceed capacity. > TimeSeries allows a window of values to be stored, while "sparsifying" older > values when the capacity is exceeded. > See the documentation on TimeSeries for details. > > An alternative, more deterministic approach, is to use sampling on older > values. > Both approaches have their advantages and disadvantages. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/statistics.hpp > ce122a5eaa1aa9c09207cc8c9428136b681561cf > 3rdparty/libprocess/src/statistics.cpp > d4ba9f146f7b9b46525a0e27fbfb3d61a21a94fc > 3rdparty/libprocess/src/tests/statistics_tests.cpp > e6c9a1b776d6f3339f96898c3501d2a00c416006 > > Diff: https://reviews.apache.org/r/13603/diff/ > > > Testing > ------- > > Added tests to verify the truncation and sparsification of time series. > > > Thanks, > > Ben Mahler > >
