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