> On April 23, 2014, 11:17 a.m., Ben Mahler wrote: > > src/master/registrar.cpp, lines 148-151 > > <https://reviews.apache.org/r/20581/diff/4/?file=565299#file565299line148> > > > > Would love to see some structs here (we can name them and add > > constructors if necessary) > > > > struct Timers > > { > > Timer state_fetch; > > Timer state_store; > > } timers; > > > > struct Gauges > > { > > Gauge queued_operations; > > } gauges; > > > > It seems that we should make an exception here and use snake_case so > > that we match the names of the metrics as they are exposed in the endpoint? > > Dominic Hamon wrote: > This slightly complicates the gauge construction as the struct doesn't > have access to the access method for the operations queue size. I'll put up a > patch with this so you can see what's necessary and compare. > > Also - naming exceptions as a policy for metrics seem like the thin end > of a wedge. I don't think it's onerous to search for a metric name and find > the variable being constructed. > > Ben Mahler wrote: > Ok will take a look. > > As for naming, it's important for us to consider the impact on the > understandability of our system. I think for metrics, it is perfectly > reasonable to make these match the external metric names. We considered this > in a similar way when we were designing the cgroups::memory library. Is it > clear to read cgroups::memory::limit_in_bytes and realize that this is > touching the "limit_in_bytes" file? I think so. > > In this case, I think it's more clear to read "timers.state_fetch" and be > able to directly map that to the metric name, rather than requiring a mental > level of indirection in order to map "stateFetch" to "state_fetch". I would > prefer clearer code that makes exceptions to rules than compromising clarity > in favor of always following rules. > > As for thin edge of a wedge, that's what code reviews are for! We'll make > sure rules are broken in reasonable ways. :) > > Does this make sense to you Dominic?
+1 i'll use the snake case names. i won't update the patch until you comment on the structs to avoid noise. - Dominic ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20581/#review41173 ----------------------------------------------------------- On April 23, 2014, 11:44 a.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20581/ > ----------------------------------------------------------- > > (Updated April 23, 2014, 11:44 a.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 > >
