----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19504/#review41973 -----------------------------------------------------------
Could we have a small integration test that starts a Master and ensures the right names are present in /stats.json and /metrics/snapshot? I think the comments I made here would generalize to the slave patch as well, could you take them into account there as well? src/master/http.cpp <https://reviews.apache.org/r/19504/#comment75653> Any plan to follow up for this one later? (Would be nice to have a complete deprecation for all old statistics in 0.19.0, without these we aren't offering the data in both endpoints). src/master/master.hpp <https://reviews.apache.org/r/19504/#comment75658> Should all of these be 'const'? src/master/master.hpp <https://reviews.apache.org/r/19504/#comment75655> Is it possible to move this one up into the header? src/master/master.hpp <https://reviews.apache.org/r/19504/#comment75654> Could we rename these to match the exposed metric's names? (Similar to how the one's below are named, e.g. "dropped_messages" over "droppedMessages"). s/isElected/elected/ as well? Ditto for the gauge methods themselves (akin to how we named RegistrarProcess::_queued_operations). src/master/master.cpp <https://reviews.apache.org/r/19504/#comment75657> Can we just keep this in the header as before so it's all in one place for now? We could think of ways to clean things up later :) src/master/master.cpp <https://reviews.apache.org/r/19504/#comment75656> How about just placing each argument on a newline so that this is a bit easier on the eyes? E.g. invalid_framework_messages( "master/invalid_framework_messages"), uptime( "master/uptime", defer(master, &Master::_uptime)), elected( "master/elected", defer(master, &Master::_elected)), total_schedulers( "master/total_schedulers", defer(master, &Master::_totalSchedulers)), ... dropped_messages( "master/dropped_messages"), - Ben Mahler On April 30, 2014, 6:11 p.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19504/ > ----------------------------------------------------------- > > (Updated April 30, 2014, 6:11 p.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-1132 > https://issues.apache.org/jira/browse/MESOS-1132 > > > Repository: mesos-git > > > Description > ------- > > see summary > > > Diffs > ----- > > src/master/http.cpp f54c841d003d7a4d2eda5e93405afa358707f4e0 > src/master/master.hpp 333e37ccd5746c5026740cfcb816499cea61a545 > src/master/master.cpp f205dca43f10697862e3fd3f435f1127a9d0aecb > > Diff: https://reviews.apache.org/r/19504/diff/ > > > Testing > ------- > > ran locally and checked /metrics/snapshot vs master/stats.json > > make check > > > Thanks, > > Dominic Hamon > >
