-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44694/#review123077
-----------------------------------------------------------




src/common/type_utils.cpp (lines 155 - 182)
<https://reviews.apache.org/r/44694/#comment185199>

    Let's do the more restrictive check in the duplicate module check as 
discussed offline.



src/module/manager.hpp (line 147)
<https://reviews.apache.org/r/44694/#comment185200>

    `Duplicate` seems a little weird here. To me the word can have a bad 
connotation.
    how about `Identical` or `Same`?
    Also, what's the difference between `verify` above, and `validate` here?
    Should this function also be `verify` or is it different in some way?
    
    In your comments in the implementation you seem to use `verify`.



src/module/manager.hpp (lines 147 - 150)
<https://reviews.apache.org/r/44694/#comment185201>

    Can we add a comment here that talks about verifying that modules are the 
same to support the ability to load the multiple times?
    
    Please also add a `TODO` for the work around forcing module writers to 
express whether their modules are (1) multi-instantiable, and (2) thread safe.
    
    Right now it's a guessing game, and could lead to hard to debug errors.



src/module/manager.hpp (lines 152 - 170)
<https://reviews.apache.org/r/44694/#comment185202>

    Is there an explicit reason these non-`pod` types are not indirected?
    The pattern for globals in mesos is that they are indirected to avoid 
non-deterministic finalization problems.
    
    Can you add a `TODO` here, and follow up with a patch to clean this up. If 
there is an explicit reason can you please document it?



src/module/manager.cpp (line 22)
<https://reviews.apache.org/r/44694/#comment185203>

    reminder to cut this out after re-organizing the `==` operator.



src/module/manager.cpp (lines 50 - 52)
<https://reviews.apache.org/r/44694/#comment185204>

    Why the re-ordering here?



src/module/manager.cpp (line 200)
<https://reviews.apache.org/r/44694/#comment185205>

    Is this an internal invariant?



src/module/manager.cpp (lines 208 - 217)
<https://reviews.apache.org/r/44694/#comment185206>

    will check this after you update this logic.



src/module/manager.cpp (line 219)
<https://reviews.apache.org/r/44694/#comment185207>

    backticks around `ModuleBase`



src/module/manager.cpp (line 234)
<https://reviews.apache.org/r/44694/#comment185208>

    do we want the trailing `;`?



src/module/manager.cpp (lines 287 - 294)
<https://reviews.apache.org/r/44694/#comment185209>

    Can you add a quick comment as to why we verify compatibility before we 
check for duplicates?



src/module/manager.cpp (lines 296 - 297)
<https://reviews.apache.org/r/44694/#comment185210>

    leave a blank line after statements that don't fit on a single line.



src/module/manager.cpp (line 300)
<https://reviews.apache.org/r/44694/#comment185211>

    new line after closing brace of an `if`


- Joris Van Remoortere


On March 11, 2016, 2:51 a.m., Kapil Arya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44694/
> -----------------------------------------------------------
> 
> (Updated March 11, 2016, 2:51 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere, Michael Park, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-4903
>     https://issues.apache.org/jira/browse/MESOS-4903
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> As long as the module manifest(s) being loaded don't conflict with the
> already loaded manifests, the module manager will allow multiple calls
> to `load()`.
> 
> 
> Diffs
> -----
> 
>   src/module/manager.hpp 89dd06097a355c6455e8f8af7d3d98847a304c90 
>   src/module/manager.cpp 6ae99504005581b22a44768949b1d305cec517d9 
>   src/tests/module_tests.cpp 497ed87bddce23bb457373da40dc87111c04136d 
> 
> Diff: https://reviews.apache.org/r/44694/diff/
> 
> 
> Testing
> -------
> 
> Added new tests and ran make check.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Reply via email to