----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20018/#review41077 -----------------------------------------------------------
Ship it! Alright, I'll get this committed! I made a number of comments for cleanup below but I didn't open them as "issues" since I've included the fixes in the commit. Please let me know if anything was missed or if we should follow up with anything! 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/20018/#comment74461> Let's remove this since 'context' no longer exists. 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20018/#comment74463> Please no using clause for methods like this since just seeing get() out of context is a bit unintuitive. 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20018/#comment74484> You don't need EXPECT_FLOAT_EQ for integral double values, right? 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20018/#comment74478> Looks like we could simplify this test to only do what is necessary: // Ensure JSON is empty. // Add a counter and gauge. // Ensure they are present in the JSON. // Remove them. // Ensure they are not present in the JSON. 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20018/#comment74475> Please favor names like "counter" and "gauge" over "c" and "g". 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20018/#comment74477> It seems like all of our tests in libprocess would need this, no? 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20018/#comment74476> Testing this seems unnecessary, let's opt to keep the test simple instead! 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20018/#comment74471> s/c/counter/ 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20018/#comment74465> You can remove the need for this call to advance outside the loop by advancing _before_ the counter pre-increment in the loop. 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20018/#comment74470> Advancing the clock after incrementing does not achieve anything, so does it makes sense to only increment before? 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20018/#comment74469> Let's move the response down to below the expected object. 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20018/#comment74468> Let's add some newlines here to make it easier to read. - Ben Mahler On April 22, 2014, 7:56 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, 7:56 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/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 > >
