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 Wed, 30 Sep 2015 09:55:19 GMT

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


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