You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Dominic Hamon <dh...@twopensource.com> on 2014/04/23 00:35:28 UTC
Review Request 20581: Added Metrics to registrar.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20581/
-----------------------------------------------------------
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
Re: Review Request 20581: Added metrics to registrar.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On April 22, 2014, 6:10 p.m., Ben Mahler wrote:
> > src/master/registrar.cpp, lines 87-88
> > <https://reviews.apache.org/r/20581/diff/3/?file=564996#file564996line87>
> >
> > Looks like this needs a rebase, but how about introducing an unnamed struct in Registrar so we can do:
> >
> > timers.fetch
> > timers.store
> >
> > gauges.queued_operations
> >
> > Akin to what we did for 'stats' and 'slaves' in Master.
It adds complications: It would need a default constructor, and then the Gauge is no longer defering to 'self()' but to the outer process.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20581/#review41104
-----------------------------------------------------------
On April 22, 2014, 8:03 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20581/
> -----------------------------------------------------------
>
> (Updated April 22, 2014, 8:03 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
>
>
Re: Review Request 20581: Added Metrics to registrar.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20581/#review41104
-----------------------------------------------------------
src/master/registrar.cpp
<https://reviews.apache.org/r/20581/#comment74514>
Looks like this needs a rebase, but how about introducing an unnamed struct in Registrar so we can do:
timers.fetch
timers.store
gauges.queued_operations
Akin to what we did for 'stats' and 'slaves' in Master.
- Ben Mahler
On April 23, 2014, 12:41 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, 12:41 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
>
>
Re: Review Request 20581: Added metrics to registrar.
Posted by Ben Mahler <be...@gmail.com>.
> On April 23, 2014, 6:17 p.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.
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?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20581/#review41173
-----------------------------------------------------------
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
>
>
Re: Review Request 20581: Added metrics to registrar.
Posted by Dominic Hamon <dh...@twopensource.com>.
> 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
>
>
Re: Review Request 20581: Added metrics to registrar.
Posted by Dominic Hamon <dh...@twopensource.com>.
> 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?
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.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20581/#review41173
-----------------------------------------------------------
On April 22, 2014, 8:03 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20581/
> -----------------------------------------------------------
>
> (Updated April 22, 2014, 8:03 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
>
>
Re: Review Request 20581: Added metrics to registrar.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20581/#review41173
-----------------------------------------------------------
src/master/registrar.cpp
<https://reviews.apache.org/r/20581/#comment74580>
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?
- Ben Mahler
On April 23, 2014, 3:03 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, 3:03 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
>
>
Re: Review Request 20581: Added metrics to registrar.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20581/#review41210
-----------------------------------------------------------
src/master/registrar.cpp
<https://reviews.apache.org/r/20581/#comment74648>
Could we take a const reference here?
- 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
>
>
Re: Review Request 20581: Added metrics to registrar.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On April 23, 2014, 3:06 p.m., Dominic Hamon wrote:
> > src/master/registrar.cpp, line 152
> > <https://reviews.apache.org/r/20581/diff/6/?file=566107#file566107line152>
> >
> > is 2 weeks about right for the state store timer history? or would a couple of days be more reasonable?
>
> Ben Mahler wrote:
> Yeah it sounds good since the implicit maximum would be 1000 entries, right?
>
> This means we're likely to start sparsifying significantly for a long-running master, which is ok.
>
> Dominic Hamon wrote:
> correct, the capacity will be 1000 entries so we may sparsify. I want to make sure the inertia around aggregates isn't too large so we notice when/if stores start taking longer.
>
> Ben Mahler wrote:
> That's an excellent point! Longer term do we want to separate the notion of time series history from aggregate history? I remember you mentioned differentiating aggregates on standard window sizes, like 5min, 10min, 1hr, ...?
>
> But for now, how about we make this smaller, say... 1 day?
that was when we had the idea that the time series window would be set on 'add' and kept in the MetricsProcess; it would allow the same Metric to have multiple histories.
However, we still can consider separating the aggregate from the time series - especially if/when we get around to introducing the histogram abstraction.
1 day it is.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20581/#review41228
-----------------------------------------------------------
On April 23, 2014, 1:48 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, 1:48 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
>
>
Re: Review Request 20581: Added metrics to registrar.
Posted by Ben Mahler <be...@gmail.com>.
> On April 23, 2014, 10:06 p.m., Dominic Hamon wrote:
> > src/master/registrar.cpp, line 152
> > <https://reviews.apache.org/r/20581/diff/6/?file=566107#file566107line152>
> >
> > is 2 weeks about right for the state store timer history? or would a couple of days be more reasonable?
>
> Ben Mahler wrote:
> Yeah it sounds good since the implicit maximum would be 1000 entries, right?
>
> This means we're likely to start sparsifying significantly for a long-running master, which is ok.
>
> Dominic Hamon wrote:
> correct, the capacity will be 1000 entries so we may sparsify. I want to make sure the inertia around aggregates isn't too large so we notice when/if stores start taking longer.
That's an excellent point! Longer term do we want to separate the notion of time series history from aggregate history? I remember you mentioned differentiating aggregates on standard window sizes, like 5min, 10min, 1hr, ...?
But for now, how about we make this smaller, say... 1 day?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20581/#review41228
-----------------------------------------------------------
On April 23, 2014, 8:48 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, 8:48 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
>
>
Re: Review Request 20581: Added metrics to registrar.
Posted by Ben Mahler <be...@gmail.com>.
> On April 23, 2014, 10:06 p.m., Dominic Hamon wrote:
> > src/master/registrar.cpp, line 152
> > <https://reviews.apache.org/r/20581/diff/6/?file=566107#file566107line152>
> >
> > is 2 weeks about right for the state store timer history? or would a couple of days be more reasonable?
Yeah it sounds good since the implicit maximum would be 1000 entries, right?
This means we're likely to start sparsifying significantly for a long-running master, which is ok.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20581/#review41228
-----------------------------------------------------------
On April 23, 2014, 8:48 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, 8:48 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
>
>
Re: Review Request 20581: Added metrics to registrar.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On April 23, 2014, 3:06 p.m., Dominic Hamon wrote:
> > src/master/registrar.cpp, line 152
> > <https://reviews.apache.org/r/20581/diff/6/?file=566107#file566107line152>
> >
> > is 2 weeks about right for the state store timer history? or would a couple of days be more reasonable?
>
> Ben Mahler wrote:
> Yeah it sounds good since the implicit maximum would be 1000 entries, right?
>
> This means we're likely to start sparsifying significantly for a long-running master, which is ok.
correct, the capacity will be 1000 entries so we may sparsify. I want to make sure the inertia around aggregates isn't too large so we notice when/if stores start taking longer.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20581/#review41228
-----------------------------------------------------------
On April 23, 2014, 1:48 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, 1:48 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
>
>
Re: Review Request 20581: Added metrics to registrar.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20581/#review41228
-----------------------------------------------------------
src/master/registrar.cpp
<https://reviews.apache.org/r/20581/#comment74665>
is 2 weeks about right for the state store timer history? or would a couple of days be more reasonable?
- Dominic Hamon
On April 23, 2014, 1:48 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, 1:48 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
>
>
Re: Review Request 20581: Added metrics to registrar.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On April 23, 2014, 2:52 p.m., Ben Mahler wrote:
> > src/master/registrar.cpp, lines 168-172
> > <https://reviews.apache.org/r/20581/diff/6/?file=566107#file566107line168>
> >
> > If Gauge is passed the RegistrarProcess, could it not store it and provide it's own function for getting the queue size? (Much like what we did for Master::Http).
this is complicated - we'd have to get the lock on the registrar process or somehow defer to it anyway.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20581/#review41214
-----------------------------------------------------------
On April 23, 2014, 1:48 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, 1:48 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
>
>
Re: Review Request 20581: Added metrics to registrar.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20581/#review41214
-----------------------------------------------------------
src/master/registrar.cpp
<https://reviews.apache.org/r/20581/#comment74649>
Is it necessary to invert the order here? Maybe it's just simpler to keep the same order?
src/master/registrar.cpp
<https://reviews.apache.org/r/20581/#comment74656>
If Gauge is passed the RegistrarProcess, could it not store it and provide it's own function for getting the queue size? (Much like what we did for Master::Http).
- Ben Mahler
On April 23, 2014, 8:48 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, 8:48 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
>
>
Re: Review Request 20581: Added metrics to registrar.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20581/#review41246
-----------------------------------------------------------
Ship it!
Ship It!
- Ben Mahler
On April 23, 2014, 10:28 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, 10:28 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
>
>
Re: Review Request 20581: Added metrics to registrar.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20581/
-----------------------------------------------------------
(Updated April 23, 2014, 3:28 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 (updated)
-----
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
Re: Review Request 20581: Added metrics to registrar.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20581/
-----------------------------------------------------------
(Updated April 23, 2014, 1:48 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 (updated)
-----
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
Re: Review Request 20581: Added metrics to registrar.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On April 23, 2014, 12:52 p.m., Ben Mahler wrote:
> > src/master/registrar.cpp, lines 384-385
> > <https://reviews.apache.org/r/20581/diff/5/?file=565979#file565979line384>
> >
> > Cool, it looks like the notion of timing a Future would have been useful for these cases as well!
yeah - when i started adding these i understood why that async op timer was important.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20581/#review41189
-----------------------------------------------------------
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
>
>
Re: Review Request 20581: Added metrics to registrar.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 20581: Added metrics to registrar.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
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 (updated)
-----
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
Re: Review Request 20581: Added metrics to registrar.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20581/
-----------------------------------------------------------
(Updated April 22, 2014, 8:03 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
rebased, changed names, fixed indentation.
Summary (updated)
-----------------
Added metrics to registrar.
Repository: mesos-git
Description
-------
Added Timers for state store/fetch and a Gauge to track the length of the operations queue.
Diffs (updated)
-----
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
Re: Review Request 20581: Added Metrics to registrar.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20581/
-----------------------------------------------------------
(Updated April 22, 2014, 5:41 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
rebased to master
Repository: mesos-git
Description
-------
Added Timers for state store/fetch and a Gauge to track the length of the operations queue.
Diffs (updated)
-----
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
Re: Review Request 20581: Added Metrics to registrar.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20581/
-----------------------------------------------------------
(Updated April 22, 2014, 3:51 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
removed operations timer.
Repository: mesos-git
Description
-------
Added Timers for state store/fetch and a Gauge to track the length of the operations queue.
Diffs (updated)
-----
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