----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18718/#review36071 -----------------------------------------------------------
src/metrics/counter.hpp <https://reviews.apache.org/r/18718/#comment66904> kill new line. src/metrics/counter.hpp <https://reviews.apache.org/r/18718/#comment66906> { on new line. src/metrics/counter.hpp <https://reviews.apache.org/r/18718/#comment66907> formatting. one arg per line. src/metrics/counter.hpp <https://reviews.apache.org/r/18718/#comment66908> s/initialValue/_value/ s/double()/0/ ? src/metrics/counter.hpp <https://reviews.apache.org/r/18718/#comment66909> s/value_/value/ src/metrics/counter.hpp <https://reviews.apache.org/r/18718/#comment66911> s/inc/increment/ s/dec/decrement/ src/metrics/counter.hpp <https://reviews.apache.org/r/18718/#comment66912> s/incBy/increment/ s/decBy/decrement/ src/metrics/counter.hpp <https://reviews.apache.org/r/18718/#comment66905> kill new line. src/metrics/gauge.hpp <https://reviews.apache.org/r/18718/#comment66914> kill NL. src/metrics/gauge.hpp <https://reviews.apache.org/r/18718/#comment66916> { on new line. src/metrics/gauge.hpp <https://reviews.apache.org/r/18718/#comment66915> s/TODO/TODO(dhamon)/ ? :) Also, can you expand on the TODO? src/metrics/gauge.hpp <https://reviews.apache.org/r/18718/#comment66919> s/getFn/_f/ src/metrics/gauge.hpp <https://reviews.apache.org/r/18718/#comment66918> s/getFn_/f/ ? src/metrics/gauge.hpp <https://reviews.apache.org/r/18718/#comment66913> NL. src/metrics/metric.hpp <https://reviews.apache.org/r/18718/#comment66923> s/context_/context/ s/name_/name/ src/metrics/metric.cpp <https://reviews.apache.org/r/18718/#comment66925> kill NL. src/metrics/metric.cpp <https://reviews.apache.org/r/18718/#comment66924> kill NL. src/metrics/metrics.hpp <https://reviews.apache.org/r/18718/#comment66926> Do you want to expose this to users of metrics? If not, instead of a metrics namespace how about an "internal" namespace under "mesos { metric { ". This has been our convention in the code. It would also eliminate the need to spread this code across 4 files. src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment66929> s/metrics_/metrics/ src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment66927> return Error( "Metric..."); src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment66928> formatting. src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment66941> The naming of the variables here is a bit confusing, though I don't have a good suggestion here. src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment66931> CHECK_NOTNULL(metric) src/tests/metrics_tests.cpp <https://reviews.apache.org/r/18718/#comment66943> Have you considered increment and decrement to return the current value instead of void? src/tests/metrics_tests.cpp <https://reviews.apache.org/r/18718/#comment66949> I see now why you didn't make these internal. Also, can you add tests for correct add and remove? - Vinod Kone On March 4, 2014, 12:33 a.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18718/ > ----------------------------------------------------------- > > (Updated March 4, 2014, 12:33 a.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 > ----- > > src/Makefile.am 61d832b89132be2cc5b8ae9bbf743685464f78a4 > src/metrics/counter.hpp PRE-CREATION > src/metrics/gauge.hpp PRE-CREATION > src/metrics/metric.hpp PRE-CREATION > src/metrics/metric.cpp PRE-CREATION > src/metrics/metrics.hpp PRE-CREATION > src/metrics/metrics.cpp PRE-CREATION > src/tests/metrics_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/18718/diff/ > > > Testing > ------- > > Added unit tests. make check. > > > Thanks, > > Dominic Hamon > >
