----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20581/#review41189 -----------------------------------------------------------
src/master/registrar.cpp <https://reviews.apache.org/r/20581/#comment74609> Alphabetical please. src/master/registrar.cpp <https://reviews.apache.org/r/20581/#comment74610> These being above the continuations is actually an artifact of the fact that we used to have a struct up here for slave operations: https://github.com/apache/mesos/blob/0.18.0-rc6/src/master/registrar.cpp#L162 Let's move them down below the continuations. src/master/registrar.cpp <https://reviews.apache.org/r/20581/#comment74626> On the note of clarity, I like the idea of separating these structs to make it clear when reading code what type of metric is being used: counters.total_operations timers.state_fetch vs. metrics.counters.total_operations metrics.timers.state_fetch vs. metrics.total_operations metrics.state_fetch (1) and (3) seem reasonable, it seems nice to be able to directly see the types of metrics we're working with so (1) seems to be easier to read and understand. In this code with so few metrics it probably seems unnecessary, but in places like the Master these will definitely get unwieldy if entirely contained under 'metrics.', so consistently exposing metrics under separate structs seems like a nice pattern! In general though, it's interesting to see these patterns emerge and it might be nice to take a step back after the dust settles on 0.19.0 and look at if there's anything we can do to simplify the way the API is used by callers. src/master/registrar.cpp <https://reviews.apache.org/r/20581/#comment74611> Same line for the braces. src/master/registrar.cpp <https://reviews.apache.org/r/20581/#comment74627> Could manage the lifetime during construction and destruction of the struct rather than with explicit 'add' and 'remove' struct methods? src/master/registrar.cpp <https://reviews.apache.org/r/20581/#comment74608> How about a comment above this: // Gauge handlers. src/master/registrar.cpp <https://reviews.apache.org/r/20581/#comment74625> Cool, it looks like the notion of timing a Future would have been useful for these cases as well! - Ben Mahler On April 23, 2014, 6:44 p.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20581/ > ----------------------------------------------------------- > > (Updated April 23, 2014, 6:44 p.m.) > > > Review request for mesos and Ben Mahler. > > > Repository: mesos-git > > > Description > ------- > > Added Timers for state store/fetch and a Gauge to track the length of the > operations queue. > > > Diffs > ----- > > src/master/registrar.cpp 38040bdb519c7f0562ddfeb58d2809cffa6f4c18 > > Diff: https://reviews.apache.org/r/20581/diff/ > > > Testing > ------- > > local build + mesos-local.sh + manual inspection. > make check. > > > Thanks, > > Dominic Hamon > >
