> On March 3, 2014, 5:28 p.m., Vinod Kone wrote: > > src/metrics/counter.hpp, line 35 > > <https://reviews.apache.org/r/18718/diff/1/?file=509261#file509261line35> > > > > s/initialValue/_value/ > > > > s/double()/0/ ?
i prefer double() because at some point this might become T(). > On March 3, 2014, 5:28 p.m., Vinod Kone wrote: > > src/metrics/counter.hpp, line 36 > > <https://reviews.apache.org/r/18718/diff/1/?file=509261#file509261line36> > > > > s/value_/value/ i prefer to use _ for member variables as per google-style. this makes it clear at a glance to see which are members and which are from local scope. any chance we can adopt this? > On March 3, 2014, 5:28 p.m., Vinod Kone wrote: > > src/metrics/gauge.hpp, line 35 > > <https://reviews.apache.org/r/18718/diff/1/?file=509262#file509262line35> > > > > s/TODO/TODO(dhamon)/ ? :) > > > > Also, can you expand on the TODO? removing the TODO because i'm not sure what's necessary here. I'll wait to see what the use cases are before worrying about it. > On March 3, 2014, 5:28 p.m., Vinod Kone wrote: > > src/metrics/metric.hpp, line 37 > > <https://reviews.apache.org/r/18718/diff/1/?file=509263#file509263line37> > > > > s/context_/context/ > > s/name_/name/ > > see above discussion. > On March 3, 2014, 5:28 p.m., Vinod Kone wrote: > > src/metrics/metrics.hpp, line 31 > > <https://reviews.apache.org/r/18718/diff/1/?file=509265#file509265line31> > > > > 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. I don't mind spreading the code across files, because I've found one class per file to be more readable and maintainable. Adding an internal namespace seems reasonable. > On March 3, 2014, 5:28 p.m., Vinod Kone wrote: > > src/metrics/metrics.cpp, line 35 > > <https://reviews.apache.org/r/18718/diff/1/?file=509266#file509266line35> > > > > s/metrics_/metrics/ I can't name it the same as the namespace, and this marks it as being 'non-local' which i find useful for readability. > On March 3, 2014, 5:28 p.m., Vinod Kone wrote: > > src/metrics/metrics.cpp, lines 73-76 > > <https://reviews.apache.org/r/18718/diff/1/?file=509266#file509266line73> > > > > The naming of the variables here is a bit confusing, though I don't > > have a good suggestion here. > > tried something different > On March 3, 2014, 5:28 p.m., Vinod Kone wrote: > > src/metrics/metrics.cpp, line 76 > > <https://reviews.apache.org/r/18718/diff/1/?file=509266#file509266line76> > > > > CHECK_NOTNULL(metric) Also added in the 'add' call. > On March 3, 2014, 5:28 p.m., Vinod Kone wrote: > > src/tests/metrics_tests.cpp, line 41 > > <https://reviews.apache.org/r/18718/diff/1/?file=509267#file509267line41> > > > > Have you considered increment and decrement to return the current value > > instead of void? I didn't. Would you expect pre- or post-increment semantics? And what do you see the use-case being? The counter should be the source of truth for the value. > On March 3, 2014, 5:28 p.m., Vinod Kone wrote: > > src/tests/metrics_tests.cpp, lines 66-68 > > <https://reviews.apache.org/r/18718/diff/1/?file=509267#file509267line66> > > > > I see now why you didn't make these internal. > > > > Also, can you add tests for correct add and remove? Well, I can still make them internal. They're still accessible :) It's tricky - If I instantiate a Metric it will be added automatically, so that code path is being exercised already just by creating a Counter or a Gauge. I can't think of a way to create a Metric that doesn't get added so I can add it manually. - Dominic ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18718/#review36071 ----------------------------------------------------------- On March 4, 2014, 10:50 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, 10:50 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 > >
