mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dominic Hamon" <dha...@twopensource.com>
Subject Re: Review Request 20581: Added metrics to registrar.
Date Wed, 23 Apr 2014 19:19:41 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message