On Fri, May 2, 2014 at 10:46 AM, Ben Mahler <[email protected]>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 > > > > > >
