You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2014/05/02 01:23:05 UTC
Re: Review Request 19504: Ported master stats to use new metrics library.
-----------------------------------------------------------
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
>
>
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On May 1, 2014, 4:23 p.m., Ben Mahler wrote:
> > 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?
I can add that test - given that we have tests that already check if a metric shows up in the endpoint when it's added to the metrics system I'm not sure what it would add to our test coverage. I'm also concerned that it might be fragile as it would need to be updated as people start adding and removing metrics. I may be missing something: what do you expect it to show us over what we already test?
Yes - i'll apply anything relevant to the Slave patch.
> On May 1, 2014, 4:23 p.m., Ben Mahler wrote:
> > src/master/http.cpp, line 380
> > <https://reviews.apache.org/r/19504/diff/7/?file=571713#file571713line380>
> >
> > 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).
Yes, absolutely. I wanted the metrics library to settle and these patches to land so I had a better idea of what I'd need to do to port them. I already had the slave and master patches out and wanted to avoid making similar changes to each.
> On May 1, 2014, 4:23 p.m., Ben Mahler wrote:
> > src/master/master.hpp, lines 522-534
> > <https://reviews.apache.org/r/19504/diff/7/?file=571714#file571714line522>
> >
> > 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).
Absolutely. I'm catching up with the rules we've established so far :)
Thanks for catching this.
> On May 1, 2014, 4:23 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 3679
> > <https://reviews.apache.org/r/19504/diff/7/?file=571715#file571715line3679>
> >
> > 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 :)
at nearly 60 lines of (frankly) ugly looking repetitive code, i think it clutters up an already complicated class header too much. I'd much rather keep it out - the details of what it's doing aren't particularly challenging to guess so having it in the header doesn't aid the user of Master at all.
> On May 1, 2014, 4:23 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 3680-3703
> > <https://reviews.apache.org/r/19504/diff/7/?file=571715#file571715line3680>
> >
> > 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"),
> >
done - i'm not sure it's much better to read, but i have tried a few ways and nothing really works :/
> On May 1, 2014, 4:23 p.m., Ben Mahler wrote:
> > src/master/master.hpp, line 392
> > <https://reviews.apache.org/r/19504/diff/7/?file=571714#file571714line392>
> >
> > Is it possible to move this one up into the header?
yes, if i move struct Framework {...}; above Master (or into its own header (hint hint)).
until then, it can be in the header but not within Master's class definition. Given no other methods are in the header but outside the class definition, I'll keep it in the source file for consistency.
> On May 1, 2014, 4:23 p.m., Ben Mahler wrote:
> > src/master/master.hpp, lines 377-392
> > <https://reviews.apache.org/r/19504/diff/7/?file=571714#file571714line377>
> >
> > Should all of these be 'const'?
yes - but Gauge doesn't support that. I considered having Gauge require the functions passed in to be <Future<double> void() const> but felt it would be too restrictive for users. There might be cases that users want to, say, acquire a lock before accessing data. That would then require mutable and that way madness lies!
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/#review41973
-----------------------------------------------------------
On May 1, 2014, 6:44 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19504/
> -----------------------------------------------------------
>
> (Updated May 1, 2014, 6:44 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
>
>
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On May 1, 2014, 4:23 p.m., Ben Mahler wrote:
> > src/master/master.hpp, line 392
> > <https://reviews.apache.org/r/19504/diff/7/?file=571714#file571714line392>
> >
> > Is it possible to move this one up into the header?
>
> Dominic Hamon wrote:
> yes, if i move struct Framework {...}; above Master (or into its own header (hint hint)).
>
> until then, it can be in the header but not within Master's class definition. Given no other methods are in the header but outside the class definition, I'll keep it in the source file for consistency.
>
> Ben Mahler wrote:
> That seems odd, even though 'Framework' is defined in the header below 'Master', it is forward declared at the top, which should solve this issue? What was the compiler error?
the implementation of _activeTasks (_active_tasks) requires Framework to be complete as it accesses members. We could just move Framework to above Master in the header.
> On May 1, 2014, 4:23 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 3679
> > <https://reviews.apache.org/r/19504/diff/7/?file=571715#file571715line3679>
> >
> > 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 :)
>
> Dominic Hamon wrote:
> at nearly 60 lines of (frankly) ugly looking repetitive code, i think it clutters up an already complicated class header too much. I'd much rather keep it out - the details of what it's doing aren't particularly challenging to guess so having it in the header doesn't aid the user of Master at all.
>
> Ben Mahler wrote:
> I would think less about the "user" of Master, because we don't "use" Master as a library. Rather I would think about the "reader" of the code, who's trying to understand how the Master works, what kinds of things are exposed. That being said, you're right that this is quite a bit of code bloat and could go into the source file, my only point would be that it's nice for the "reader" of the code to be able to see all of the metrics names in the header.
>
> Up to you!
Wrote this in email but it didn't show up here so, for posterity:
?That's a really good point regarding the metrics names that end up being published in the endpoint. As long as we're consistent about naming the metric variables the same as their eventual endpoint names, I'm comfortable that readers of the code won't be confused, and having the methods in the source make it simpler to see which metrics are being exposed. We just need to be conscious that we've established this precedent for naming :)
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/#review41973
-----------------------------------------------------------
On May 1, 2014, 6:44 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19504/
> -----------------------------------------------------------
>
> (Updated May 1, 2014, 6:44 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
>
>
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Dominic Hamon <dh...@twopensource.com>.
On Fri, May 2, 2014 at 10:46 AM, Ben Mahler <be...@gmail.com>wrote:
>
>
> > On May 1, 2014, 11:23 p.m., Ben Mahler wrote:
> > > src/master/master.hpp, line 392
> > > <
> https://reviews.apache.org/r/19504/diff/7/?file=571714#file571714line392>
> > >
> > > Is it possible to move this one up into the header?
> >
> > Dominic Hamon wrote:
> > yes, if i move struct Framework {...}; above Master (or into its own
> header (hint hint)).
> >
> > until then, it can be in the header but not within Master's class
> definition. Given no other methods are in the header but outside the class
> definition, I'll keep it in the source file for consistency.
>
> That seems odd, even though 'Framework' is defined in the header below
> 'Master', it is forward declared at the top, which should solve this issue?
> What was the compiler error?
>
The implementation of the method needs Framework to be a complete type as
it is accessing members. If you'd rather I just move Framework above Master
and remove the forward declaration, I can.
>
>
> > On May 1, 2014, 11:23 p.m., Ben Mahler wrote:
> > > src/master/master.cpp, line 3679
> > > <
> https://reviews.apache.org/r/19504/diff/7/?file=571715#file571715line3679>
> > >
> > > 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 :)
> >
> > Dominic Hamon wrote:
> > at nearly 60 lines of (frankly) ugly looking repetitive code, i
> think it clutters up an already complicated class header too much. I'd much
> rather keep it out - the details of what it's doing aren't particularly
> challenging to guess so having it in the header doesn't aid the user of
> Master at all.
>
> I would think less about the "user" of Master, because we don't "use"
> Master as a library. Rather I would think about the "reader" of the code,
> who's trying to understand how the Master works, what kinds of things are
> exposed. That being said, you're right that this is quite a bit of code
> bloat and could go into the source file, my only point would be that it's
> nice for the "reader" of the code to be able to see all of the metrics
> names in the header.
>
>
That's a really good point regarding the metrics names that end up being
published in the endpoint. As long as we're consistent about naming the
metric variables the same as their eventual endpoint names, I'm comfortable
that readers of the code won't be confused, and having the methods in the
source make it simpler to see which metrics are being exposed. We just need
to be conscious that we've established this precedent for naming :)
>
>
>
> Up to you!
>
>
> - Ben
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19504/#review41973
> -----------------------------------------------------------
>
>
> On May 2, 2014, 1:44 a.m., Dominic Hamon wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/19504/
> > -----------------------------------------------------------
> >
> > (Updated May 2, 2014, 1:44 a.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
> >
> >
>
>
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Ben Mahler <be...@gmail.com>.
> On May 1, 2014, 11:23 p.m., Ben Mahler wrote:
> > src/master/master.hpp, line 392
> > <https://reviews.apache.org/r/19504/diff/7/?file=571714#file571714line392>
> >
> > Is it possible to move this one up into the header?
>
> Dominic Hamon wrote:
> yes, if i move struct Framework {...}; above Master (or into its own header (hint hint)).
>
> until then, it can be in the header but not within Master's class definition. Given no other methods are in the header but outside the class definition, I'll keep it in the source file for consistency.
That seems odd, even though 'Framework' is defined in the header below 'Master', it is forward declared at the top, which should solve this issue? What was the compiler error?
> On May 1, 2014, 11:23 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 3679
> > <https://reviews.apache.org/r/19504/diff/7/?file=571715#file571715line3679>
> >
> > 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 :)
>
> Dominic Hamon wrote:
> at nearly 60 lines of (frankly) ugly looking repetitive code, i think it clutters up an already complicated class header too much. I'd much rather keep it out - the details of what it's doing aren't particularly challenging to guess so having it in the header doesn't aid the user of Master at all.
I would think less about the "user" of Master, because we don't "use" Master as a library. Rather I would think about the "reader" of the code, who's trying to understand how the Master works, what kinds of things are exposed. That being said, you're right that this is quite a bit of code bloat and could go into the source file, my only point would be that it's nice for the "reader" of the code to be able to see all of the metrics names in the header.
Up to you!
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/#review41973
-----------------------------------------------------------
On May 2, 2014, 1:44 a.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19504/
> -----------------------------------------------------------
>
> (Updated May 2, 2014, 1:44 a.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
>
>