You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joris Van Remoortere <jo...@gmail.com> on 2015/09/30 02:09:14 UTC
Review Request 38869: Added static->dynamic transformation to
Allocator.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38869/
-----------------------------------------------------------
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
Re: Review Request 38869: Added static->dynamic transformation to
Allocator.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38869/#review101160
-----------------------------------------------------------
Hm.. it is suprising that touching this header requires wide re-compilation. Could we audit why that is?
Looking through the includes, I can see tests/cluster.hpp pulls this in, but we should have a .cpp to avoid the dependency in the header. This should yield more dependency removals since we can cut other includes. The other culprit looks to be tests/mesos.hpp which exposes an allocator wrapper for mocking. We've been planning to split out the mocks into relevant mocks/*.hpp files, so this could be migrated to a mocks/allocator.hpp file that is only included when needed in a test. FWICT only 5 test files need this.
Doing the dependency cleanup should reduce the need for this refactor, and help speed up compilation more generally. I'm curious about the implications of this pattern for our other templated headers, so hoping to be able to avoid if possible :)
- Ben Mahler
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
>
>
Re: Review Request 38869: Added static->dynamic transformation to
Allocator.
Posted by Joris Van Remoortere <jo...@gmail.com>.
> On Sept. 30, 2015, 5:10 p.m., Joseph Wu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 19
> > <https://reviews.apache.org/r/38869/diff/2/?file=1087494#file1087494line19>
> >
> > Shouldn't this go at the bottom of the includes?
I believe we are still trying to follow the guideline of including the header that we're implementing as the first file in the include list.
I recognize this is not consistent in the code-base. Joerg is trying to fix that.
- Joris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38869/#review101127
-----------------------------------------------------------
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
>
>
Re: Review Request 38869: Added static->dynamic transformation to
Allocator.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38869/#review101127
-----------------------------------------------------------
src/master/allocator/mesos/hierarchical.cpp (line 19)
<https://reviews.apache.org/r/38869/#comment158459>
Shouldn't this go at the bottom of the includes?
- Joseph Wu
On Sept. 29, 2015, 6:08 p.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38869/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2015, 6:08 p.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
>
>
Re: Review Request 38869: Added static->dynamic transformation to
Allocator.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> 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
>
>
Re: Review Request 38869: Added static->dynamic transformation to
Allocator.
Posted by Joris Van Remoortere <jo...@gmail.com>.
> 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
>
>
Re: Review Request 38869: Added static->dynamic transformation to
Allocator.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 38869: Added static->dynamic transformation to
Allocator.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38869/#review101227
-----------------------------------------------------------
Does it make sense to make the default allocator as a module?
- Guangya Liu
On 九月 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 九月 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
>
>
Re: Review Request 38869: Added static->dynamic transformation to
Allocator.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
moved includes around.
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 (updated)
-----
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
Re: Review Request 38869: Added static->dynamic transformation to
Allocator.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38869/#review101053
-----------------------------------------------------------
src/Makefile.am (line 489)
<https://reviews.apache.org/r/38869/#comment158363>
Note that if review #38827 goes in before this, it would be great to add this line to the CMakeLists file in src/
- Alex Clemmer
On Sept. 30, 2015, 12:09 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, 12:09 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
>
>
Re: Review Request 38869: Added static->dynamic transformation to
Allocator.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38869/#review101055
-----------------------------------------------------------
Patch looks great!
Reviews applied: [38869]
All tests passed.
- Mesos ReviewBot
On Sept. 30, 2015, 12:09 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, 12:09 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
>
>