----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38869/#review101100 -----------------------------------------------------------
I like when the code is pushed to `.cpp` files : ). Just to confirm that this is our intention: with the prvious design, `RoleSorter` and `FrameworkSorter` could be anything (well, anything that can be used as a sorter), now we restrict sorters to subclass `Sorter`. I think this is fine, but would like to confirm with you. A high level question I have: why do you like to preserve template approach? I think we can require allocator writers to provide a factory with their own allocator and get rid of templates completely. What do you think? src/master/allocator/mesos/hierarchical.hpp <https://reviews.apache.org/r/38869/#comment158424> We should leave at least ``` #include <string> #include <mesos/mesos.hpp> ``` here. src/master/allocator/mesos/hierarchical.hpp (line 22) <https://reviews.apache.org/r/38869/#comment158423> I think `<process/id.hpp>` should be here. Alternatively, `HierarchicalAllocatorProcess`'s c-tor can be moved into `.cpp`. Also, though `<process/future.hpp>` pulls `<process/pid.hpp>`, I think it should be included explicitly. src/master/allocator/mesos/hierarchical.hpp <https://reviews.apache.org/r/38869/#comment158426> `<stout/hashset.hpp>` should stay here. src/master/allocator/mesos/hierarchical.hpp (line 28) <https://reviews.apache.org/r/38869/#comment158430> I think `<stout/option.hpp>` should be included here. src/master/allocator/mesos/hierarchical.hpp (line 62) <https://reviews.apache.org/r/38869/#comment158425> virtual d-tor? src/master/allocator/mesos/hierarchical.hpp (lines 91 - 95) <https://reviews.apache.org/r/38869/#comment158428> Why have you decided to leave the c-tor here and not in the cpp file? src/master/allocator/mesos/hierarchical.cpp (line 33) <https://reviews.apache.org/r/38869/#comment158427> remove this newline. src/master/allocator/mesos/hierarchical.cpp (lines 131 - 136) <https://reviews.apache.org/r/38869/#comment158429> How about pulling this code to the c-tor? this will allow us not to store a copy of `sorterFactory` in allocator. - Alexander Rukletsov On Sept. 30, 2015, 1:08 a.m., Joris Van Remoortere wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38869/ > ----------------------------------------------------------- > > (Updated Sept. 30, 2015, 1:08 a.m.) > > > Review request for mesos, Ben Mahler, Cody Maloney, Artem Harutyunyan, and > Joseph Wu. > > > Bugs: MESOS-3554 > https://issues.apache.org/jira/browse/MESOS-3554 > > > Repository: mesos > > > Description > ------- > > This improves the compilation time of Mesos significantly, allowing > developers to iterate more quickly on allocator changes. > > > Diffs > ----- > > src/Makefile.am 8aa456611dd5405336dd7b0c19ba4a942ea1c805 > src/master/allocator/mesos/hierarchical.hpp > f3a9b9d799695c11caad8ae64e1a53e08bb6e63d > src/master/allocator/mesos/hierarchical.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/38869/diff/ > > > Testing > ------- > > make check > touched hierarchical.cpp and recompiled. Verified we only rebuild the module > and relink. > > > Thanks, > > Joris Van Remoortere > >
