> On Jan. 28, 2013, 8:08 p.m., Vinod Kone wrote: > > third_party/libprocess/src/statistics.cpp, line 50 > > <https://reviews.apache.org/r/9091/diff/1/?file=251530#file251530line50> > > > > Why virtual? Are you expecting this to be subclassed?
virtual so that deletion of a Process pointer pointing to a StatisticsProcess will call this destructor. I think destructors should always be virtual when dealing with inheritance: http://www.learncpp.com/cpp-tutorial/123-virtual-destructors-virtual-assignment-and-overriding-virtualization/ > On Jan. 28, 2013, 8:08 p.m., Vinod Kone wrote: > > third_party/libprocess/src/statistics.cpp, line 139 > > <https://reviews.apache.org/r/9091/diff/1/?file=251530#file251530line139> > > > > What if context doesn't exist? The [] operator will insert it if missing, which is the desired behavior here, am I missing anything else? > On Jan. 28, 2013, 8:08 p.m., Vinod Kone wrote: > > third_party/libprocess/src/statistics.cpp, line 176 > > <https://reviews.apache.org/r/9091/diff/1/?file=251530#file251530line176> > > > > so is it ok to mutate the list and use its iterator at the same time!? This was the original code from Statistics, but reading up on the map erase: "Iterators, pointers and references referring to elements removed by the function are invalidated. All other iterators, pointers and references keep their validity." So technically, we would need to advance to the next element to avoid invalidation. (see: http://stackoverflow.com/a/5673209) But I've included a simpler fix here. > On Jan. 28, 2013, 8:08 p.m., Vinod Kone wrote: > > third_party/libprocess/src/statistics.cpp, line 173 > > <https://reviews.apache.org/r/9091/diff/1/?file=251530#file251530line173> > > > > kinda weird to the name of the iterator that traverses the whole map as > > 'start'? > > > > s/start/it? Well this iterator is always referring to the start, since we continually truncate off the first element. - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9091/#review15763 ----------------------------------------------------------- On Jan. 24, 2013, 9:06 a.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9091/ > ----------------------------------------------------------- > > (Updated Jan. 24, 2013, 9:06 a.m.) > > > Review request for mesos, Benjamin Hindman and Vinod Kone. > > > Description > ------- > > This adds context into the Statistics model. Contexts provide a clean way to > logically group stats (e.g. contexts: "master", "slave", "monitor", > "libprocess", etc). > > > 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/statistics_tests.cpp > 0aaab3526618171c7cfbd11d40d614344bcbfd0a > > Diff: https://reviews.apache.org/r/9091/diff/ > > > Testing > ------- > > make check + libprocess make check > > > Thanks, > > Ben Mahler > >
