mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joris Van Remoortere" <joris.van.remoort...@gmail.com>
Subject Re: Review Request 38869: Added static->dynamic transformation to Allocator.
Date Wed, 30 Sep 2015 21:00: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?

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


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