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