----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20339/#review41051 -----------------------------------------------------------
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/include/process/metrics/timer.hpp <https://reviews.apache.org/r/20339/#comment74480> No periods in these strings, how about "No value"? 3rdparty/libprocess/include/process/metrics/timer.hpp <https://reviews.apache.org/r/20339/#comment74481> Do we want it to be an error though? Let's kill this TODO since it deals with the potential implementation detail, and consider a TODO that motivates making these errors instead. 3rdparty/libprocess/include/process/metrics/timer.hpp <https://reviews.apache.org/r/20339/#comment74444> Do you need to push in the locked section? 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20339/#comment74435> We can just use the process namespace as done in other tests! 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20339/#comment74423> Typically we don't have using clauses for something as generic as "add" and "remove", it makes the tests here a bit less clear. 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20339/#comment74440> In general let's try to prefer meaningful names like "counter" instead of "c", "timer" instead of "t", and "gauge" instead of "g". 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20339/#comment74437> newline would have been nice here 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20339/#comment74438> ditto here 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20339/#comment74424> Missing include for duration.hpp. 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20339/#comment74441> Seems a bit cleaner to just re-use one gauge here via assignment, I'll update the test. 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20339/#comment74432> I'll update this to be s/MetricsTest/Metrics/ in this file. 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/20339/#comment74433> It looks like we could get away with a simpler test that uses 1 timer and Clock::now to ensure the relative timing is correct. - Ben Mahler On April 22, 2014, 6:35 p.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20339/ > ----------------------------------------------------------- > > (Updated April 22, 2014, 6:35 p.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Bugs: MESOS-1217 > https://issues.apache.org/jira/browse/MESOS-1217 > > > Repository: mesos-git > > > Description > ------- > > see summary. > > > Diffs > ----- > > 3rdparty/libprocess/Makefile.am 0a8a31bf107041e4dd014f81785ac27e255f29a2 > 3rdparty/libprocess/include/process/metrics/timer.hpp PRE-CREATION > 3rdparty/libprocess/src/tests/metrics_tests.cpp > abe1588c931b45a09294812974788aa74de44dd4 > > Diff: https://reviews.apache.org/r/20339/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Dominic Hamon > >
