----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20018/#review41044 -----------------------------------------------------------
Almost there! 3rdparty/libprocess/include/process/statistics.hpp <https://reviews.apache.org/r/20018/#comment74406> Looking around at other libraries, "p999" and "p9999" seem to be pretty standard so I take back that this might confuse people. Let's revert, sound good? 3rdparty/libprocess/include/process/statistics.hpp <https://reviews.apache.org/r/20018/#comment74405> Looks like there is a regression here! What happens on line 76 when position is negative? Likely the CHECK_LT on line 86 will fail! This seems more difficult to understand and this change introduces a bug, let's revert! 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20018/#comment74412> Is this rebased off master? This test looks to be already committed. 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20018/#comment74411> size_t? 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20018/#comment74413> Hm, we probably should check the other percentiles here as well, no? 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20018/#comment74414> This test is pretty difficult to digest, can you add some comments above each logical block describing what you're testing? s/JSONAddAndRemove/Snapshot/ ? 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20018/#comment74415> s/jsonResponse/expected/? 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20018/#comment74417> Is there any reason to overlap the removal of gauge and counter? Could we remove the gauge first to make this a bit easier to follow? 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20018/#comment74409> Most of our tests in libprocess (a few exceptions) omit the "Test" suffix here, can you s/MetricsTest/Metrics/? s/JSONStatistics/SnapshotSatistics/ ? 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20018/#comment74410> Increment consistently instead of pre _and_ post here? Use size_t? 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20018/#comment74408> Can we come up with an interval that generates more intuitive percentiles? One that a reader can intuit? 3rdparty/libprocess/src/tests/statistics_tests.cpp <https://reviews.apache.org/r/20018/#comment74407> Thanks! [-5,5] seems much cleaner than the extra 0 and [-5,4]! - Ben Mahler On April 22, 2014, 6:20 p.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20018/ > ----------------------------------------------------------- > > (Updated April 22, 2014, 6:20 p.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone. > > > Bugs: MESOS-1036 > https://issues.apache.org/jira/browse/MESOS-1036 > > > Repository: mesos-git > > > Description > ------- > > see summary. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/metrics/metric.hpp > 6a384ded8a4b57fd6aee819d0337773018c45669 > 3rdparty/libprocess/include/process/metrics/metrics.hpp > c20bb639e8ef79de63f0d0d56c2ea40a15a1f995 > 3rdparty/libprocess/include/process/statistics.hpp > d046bffa00fa839909112f65d19dfb9af46ad2b3 > 3rdparty/libprocess/src/metrics/metrics.cpp > 391295aea91e837bb856a40ef51d1c33d44371d8 > 3rdparty/libprocess/src/tests/metrics_tests.cpp > abe1588c931b45a09294812974788aa74de44dd4 > 3rdparty/libprocess/src/tests/statistics_tests.cpp > 478453fd60056603cf2eb96e56ac2df7e47a3e99 > > Diff: https://reviews.apache.org/r/20018/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Dominic Hamon > >
