mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexander Rukletsov" <ruklet...@gmail.com>
Subject Re: Review Request 38869: Added static->dynamic transformation to Allocator.
Date Fri, 02 Oct 2015 08:50:09 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message