----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18718/#review38544 -----------------------------------------------------------
Nice to see this working asynchronously now! Mostly just more cleanup needed to get this committed. 3rdparty/libprocess/src/metrics/metric.cpp <https://reviews.apache.org/r/18718/#comment70780> Let's make this an 'internal' namespace. Can we just call this 'fail'? Seems a bit odd that 'add' leads to a 'writeFailure'. 3rdparty/libprocess/src/metrics/metric.cpp <https://reviews.apache.org/r/18718/#comment70779> Let's add a TODO to make destruction non-blocking (possibly via having copyable reference counted Metrics akin to Future / Subprocess / etc). 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment70787> Can we have this be just 'context' like in the header? Ditto elsewhere below. 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment70783> Can you use .contains() instead of .count()? 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment70782> Do you want to close the single quote here? 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment70784> Ditto here for .contains(). 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment70785> Ditto for .contains(). 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment70788> Kill std:: and s/metricValues/values/ ? Let's try to keep the name consistent between here and the continuation, here you're passing 'metricValues' and the continuation calls this 'metrics'. Why not just call them 'values' in both places? 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment70786> Any reason we can't s/contextName/context/ and s/metricName/name/? 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment70789> Just 2 space indent here. 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment70790> Kill std:: ? 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment70791> kill std:: and let's take a const& of the Future as well. 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment70792> Kill std:: ? 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment70793> Kill anonymous namespace. 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/18718/#comment70794> newline 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/18718/#comment70799> Please keep the style consistent with the rest of our code! 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/18718/#comment70795> Please clean up this spawned process at the end of the test (wait, terminate) or ask libprocess to manage it for you (via spawn(_, true). 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/18718/#comment70796> Why the double negation? 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/18718/#comment70797> Can you use one of our AWAIT_ testing macros here? This will put an upper bound on the time the test will remain blocked. - Ben Mahler On March 25, 2014, 10:06 p.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18718/ > ----------------------------------------------------------- > > (Updated March 25, 2014, 10:06 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 3c6219eb6e76306463b3710ab7e50ec8b75d3d76 > 3rdparty/libprocess/include/process/metrics/counter.hpp PRE-CREATION > 3rdparty/libprocess/include/process/metrics/gauge.hpp PRE-CREATION > 3rdparty/libprocess/include/process/metrics/metric.hpp PRE-CREATION > 3rdparty/libprocess/src/metrics/metric.cpp PRE-CREATION > 3rdparty/libprocess/src/metrics/metrics.hpp PRE-CREATION > 3rdparty/libprocess/src/metrics/metrics.cpp PRE-CREATION > 3rdparty/libprocess/src/tests/metrics_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/18718/diff/ > > > Testing > ------- > > Added unit tests. make check. > > > Thanks, > > Dominic Hamon > >
