> 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?
> 
> Joris Van Remoortere wrote:
>     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?

I think it's fine to leave it as it is now to keep the scope of this patch 
narrow. But in the future I think it makes sense to remove the templates all 
together for simplicity.


> 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.
> 
> Joris Van Remoortere wrote:
>     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?

I see your point. I think we can change the behaviour, but I'm fine with doing 
that in a separate patch. Feel free to drop the issue!


- Alexander


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