----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20047/#review40989 -----------------------------------------------------------
Ship it! Thanks Dominic! I commented on a number of small things below that I'll get cleaned up for you. I also did some commenting and other small cleanups. Will get this committed shortly! Let me know if there's anything I've changed that you would like to follow up on! 3rdparty/libprocess/include/process/statistics.hpp <https://reviews.apache.org/r/20047/#comment74346> No need for this. 3rdparty/libprocess/include/process/statistics.hpp <https://reviews.apache.org/r/20047/#comment74336> Thinking about this a little more, let's keep it consistent with how they're exposed in the endpoint in r/20018, i.e. p999, p9999. 3rdparty/libprocess/include/process/statistics.hpp <https://reviews.apache.org/r/20047/#comment74337> If percentile_ is 1.0, the CHECK_LT below will fail! position = 1.0 * (values.size()-1) index = values.size()-1 CHECK_LT(values.size(), values.size() I'll update this to handle the edge cases explicitly instead. I'll also add a CHECK that values is not empty here. 3rdparty/libprocess/include/process/statistics.hpp <https://reviews.apache.org/r/20047/#comment74338> You need to include glog for these CHECKs. 3rdparty/libprocess/src/process.cpp <https://reviews.apache.org/r/20047/#comment74347> I trust you will follow up with the new TODOs in this file? 3rdparty/libprocess/src/tests/statistics_tests.cpp <https://reviews.apache.org/r/20047/#comment74341> Doesn't look like we need this, but we do need <gtest/gtest.h>? 3rdparty/libprocess/src/tests/statistics_tests.cpp <https://reviews.apache.org/r/20047/#comment74339> Let's just kill the test frame as it makes these tests a bit harder to understand. 3rdparty/libprocess/src/tests/statistics_tests.cpp <https://reviews.apache.org/r/20047/#comment74340> You need to include clock.hpp for this. 3rdparty/libprocess/src/tests/statistics_tests.cpp <https://reviews.apache.org/r/20047/#comment74342> Did you intend to insert two 0's? One here and one in the loop? 3rdparty/libprocess/src/tests/statistics_tests.cpp <https://reviews.apache.org/r/20047/#comment74343> You need duration.hpp for Seconds. 3rdparty/libprocess/src/timeseries.cpp <https://reviews.apache.org/r/20047/#comment74344> No indentation needed here. 3rdparty/libprocess/src/timeseries.cpp <https://reviews.apache.org/r/20047/#comment74345> Don't we need <stddef.h> for this? - Ben Mahler On April 21, 2014, 11:05 p.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20047/ > ----------------------------------------------------------- > > (Updated April 21, 2014, 11:05 p.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Bugs: MESOS-1036 > https://issues.apache.org/jira/browse/MESOS-1036 > > > Repository: mesos-git > > > Description > ------- > > see summary > > > Diffs > ----- > > 3rdparty/libprocess/Makefile.am d707ad759dacd16e0177e14f1bf5ece9e4ce2491 > 3rdparty/libprocess/include/process/statistics.hpp > a4f1db3a8a219c39193a1d237477f0350e47e681 > 3rdparty/libprocess/src/process.cpp > 9654c0437edb43cff65dbefdf08dee9e18ef96ab > 3rdparty/libprocess/src/statistics.cpp > 75aac4074d33cb5054da6c8b0bd4a890c2eaf80e > 3rdparty/libprocess/src/tests/statistics_tests.cpp > 3521bd565dae8fcbba464f2539b3b14a37a037f0 > 3rdparty/libprocess/src/timeseries.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/20047/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Dominic Hamon > >
