> On Sept. 30, 2015, 9:55 a.m., Alexander Rukletsov wrote:
> > 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?

Indeed, the old implementation was just enforcing the same interface as 
`Sorter` but through templates.
I kept the template approach because it minimizes the size and impact of this 
patch. I think we can make a separate discussion around removing the template 
all together.
All the tests, and possibly code that people have written outside of our repo 
will continue to work using the current approach as the external interface does 
not change.
I realize that people shouldn't be relying on these internal files, but I think 
we can isolate this change to just a compile time optimization, leaving out the 
API change.
What are your thoughts?


> On Sept. 30, 2015, 9:55 a.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.hpp, lines 110-114
> > <https://reviews.apache.org/r/38869/diff/2/?file=1087493#file1087493line110>
> >
> >     Why have you decided to leave the c-tor here and not in the cpp file?

The diff generated by this is rather large, and I wanted to make it very 
obvious which parts of the code are changing (The top of the diff), and make it 
as easy as possible for someone to verify that nothing else changed (i.e. the 
rest of the file was just copy pasted, with template prototypes removed).


> On Sept. 30, 2015, 9:55 a.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 131-136
> > <https://reviews.apache.org/r/38869/diff/2/?file=1087494#file1087494line131>
> >
> >     How about pulling this code to the c-tor? this will allow us not to 
> > store a copy of `sorterFactory` in allocator.

We can. I didn't want to break any assumptions between constructor vs 
initializer ordering of these events.
Would it make sense to make this a separate patch as it changes the behavior?


- Joris


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38869/#review101100
-----------------------------------------------------------


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

Reply via email to