> On Oct. 23, 2014, 12:46 p.m., Vinod Kone wrote: > > Why are some definitions in .hpp and some in .cpp? Why not all in .cpp? > > > > Also, it's not clear to me how this split would help in unit testing? > > AFAICT, all these visitors take Master or Framework or Slave which needs > > bringing up a cluster. > > Dominic Hamon wrote: > They could be all in cpp, but some are so simple that being inlineable > seemed beneficial. > > This is a first step. The Master/Slave/Frameworks passed in could be > mock/stub versions that would support lightweight testing. The change also > has a benefit in reducing the amount of code in the master source file, which > helps with compile time and readability. > > Cody Maloney wrote: > Things in cpp files can be inlined with LTO (Which is something I'd like > to get functioning in Mesos tooling in in the long term). There isn't much > code which includes the header, so the increase in compile time / object size > from having more in the header isn't something I'm worried about in this case > though, so net "meh" from me either way. > > Reducing code in master.cpp is definitely good, although most of the > slowness compiling it comes from things it includes, not master.cpp itself. > (Flags tends to be one of the worse offenders last time I did some > investigation).
Agreed on all points. The main concern for me is starting to tease apart master.cpp to make things more readable, more composable, and to start to adhere to the Single Responsibility Principle. As a side effect, we can start writing more focused unit testing rather than integration testing (ie, the validation steps can be tested in isolation rather than requiring a master/slave/framework to actually be running). This should help our test time and increase code coverage (in cases where we don't happen to test the validation, which is admittedly rare). - Dominic ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20423/#review58071 ----------------------------------------------------------- On Oct. 23, 2014, 3:48 p.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20423/ > ----------------------------------------------------------- > > (Updated Oct. 23, 2014, 3:48 p.m.) > > > Review request for mesos and Benjamin Hindman. > > > Bugs: MESOS-1064 > https://issues.apache.org/jira/browse/MESOS-1064 > > > Repository: mesos-git > > > Description > ------- > > This is the first step toward being able to write independent unit tests for > the validation visitors. > > It also uses Owned to make visitor cleanup simpler (non-existent). > > > Diffs > ----- > > 3rdparty/libprocess/include/process/timeout.hpp > 0bf63e11a7a63b714aafb5386bf2d169260396ce > src/Makefile.am 2617f77b757cb7414889520c88b1bc203dedef09 > src/master/master.cpp 95589b8f25a3e496eabc0cf84319c883c1ed7aec > src/master/offervisitor.hpp PRE-CREATION > src/master/offervisitor.cpp PRE-CREATION > src/master/taskinfovisitor.hpp PRE-CREATION > src/master/taskinfovisitor.cpp PRE-CREATION > src/slave/containerizer/mesos/containerizer.cpp > 9f745d897119a814bd9f8e1b6a0ce5eaef60ed36 > src/zookeeper/group.hpp 924613065521a72887a2721b3db89f448fa50900 > > Diff: https://reviews.apache.org/r/20423/diff/ > > > Testing > ------- > > make check. > ran Java test framework. > > > Thanks, > > Dominic Hamon > >
