----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18718/#review37749 -----------------------------------------------------------
3rdparty/libprocess/include/process/metrics/counter.hpp <https://reviews.apache.org/r/18718/#comment69396> No ASF license in libprocess please (please fix here and everywhere else in this review). We could add the Apache License headers, but then we should do this everywhere, and let's do it in a different review. 3rdparty/libprocess/include/process/metrics/counter.hpp <https://reviews.apache.org/r/18718/#comment69399> Please put a newline between these. 3rdparty/libprocess/include/process/metrics/counter.hpp <https://reviews.apache.org/r/18718/#comment69400> We indent initializer lists +2 not +4. Only argument/parameter lists are indented +4. 3rdparty/libprocess/include/process/metrics/counter.hpp <https://reviews.apache.org/r/18718/#comment69407> // __PROCESS_METRICS_COUNTER_HPP__ Here and everywhere else. 3rdparty/libprocess/include/process/metrics/gauge.hpp <https://reviews.apache.org/r/18718/#comment69420> Please put process headers before stout (just like you did in metrics.cpp below ;) ). 3rdparty/libprocess/include/process/metrics/gauge.hpp <https://reviews.apache.org/r/18718/#comment69411> This is not the style we use for public APIs in libprocess (except for Future where we've wanted this to change). Please use the type in constructor and instance field below. 3rdparty/libprocess/include/process/metrics/gauge.hpp <https://reviews.apache.org/r/18718/#comment69412> Indentation. 3rdparty/libprocess/include/process/metrics/metric.hpp <https://reviews.apache.org/r/18718/#comment69418> What's the reasoning why Metric is internal? 3rdparty/libprocess/include/process/metrics/metric.hpp <https://reviews.apache.org/r/18718/#comment69413> Kill newline. 3rdparty/libprocess/include/process/metrics/metric.hpp <https://reviews.apache.org/r/18718/#comment69415> Kill newline. 3rdparty/libprocess/src/metrics/metric.cpp <https://reviews.apache.org/r/18718/#comment69421> How about including the 'context' and 'name' so we can get a better understanding of which metric might have failed? 3rdparty/libprocess/src/metrics/metric.cpp <https://reviews.apache.org/r/18718/#comment69422> What about the failure cases? Also, it kind of looks like this is a Future<Try<Nothing>>, why not just Future<Nothing>? 3rdparty/libprocess/src/metrics/metric.cpp <https://reviews.apache.org/r/18718/#comment69428> It looks like there could be a nasty race here where the Metric gets deleted but something that still has 'this' (from the 'add' call) might try and use the now cleaned up memory. 3rdparty/libprocess/src/metrics/metrics.hpp <https://reviews.apache.org/r/18718/#comment69424> Isn't the Metric declared in 'internal'? 3rdparty/libprocess/src/metrics/metrics.hpp <https://reviews.apache.org/r/18718/#comment69430> Using a Try in the Future seems redundant and a pain for a caller (who has to check both the Future failure case and Try error case). 3rdparty/libprocess/src/metrics/metrics.hpp <https://reviews.apache.org/r/18718/#comment69444> I believe your intention to expose these is so that we can write tests where we expect certain metrics to have been set. If that's the case, these need to get pulled into public headers. Likewise, maybe rather than 'initialize' and 'shutdown' we just want the ability to clear all metrics? I think some (most) of that will happen when each Metric is destructed, but for metrics that might live past tests perhaps it makes sense to have a clear? 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment69431> Pull this one below process.hpp after a newline. 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment69433> '{' on newline. 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment69448> '{' on newline. 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment69449> '{' on newline. 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment69445> I'd like to see help for this added before we commit. 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment69450> '{' on newline. 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment69434> Why does this need to be a pointer? Also, it seems like this should be called 'contexts' and from within contexts you can get metrics. 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment69435> It's not clear that we'll need to call initialize more than once unless this is driven by a test need which can't get away with something like 'clear'. So why not use something like 'Once' and always make sure it's initialized? 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment69436> '{' on newline. 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment69437> Why was this necessary? 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment69438> '{' on newline. 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment69439> Why was this necessary? 3rdparty/libprocess/src/process.cpp <https://reviews.apache.org/r/18718/#comment69419> Pull this out below like we do in our other files. - Benjamin Hindman On March 18, 2014, 12:14 a.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18718/ > ----------------------------------------------------------- > > (Updated March 18, 2014, 12:14 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 > ----- > > 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/process.cpp > 6c6acc03ad78779e8f25b1a4584069fd377f647b > 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 > >
