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