This is an automatically generated e-mail. To reply, visit:

src/common/type_utils.cpp (lines 155 - 182)

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

src/module/manager.hpp (line 147)

    `Duplicate` seems a little weird here. To me the word can have a bad 
    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)

    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)

    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)

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

src/module/manager.cpp (lines 50 - 52)

    Why the re-ordering here?

src/module/manager.cpp (line 200)

    Is this an internal invariant?

src/module/manager.cpp (lines 208 - 217)

    will check this after you update this logic.

src/module/manager.cpp (line 219)

    backticks around `ModuleBase`

src/module/manager.cpp (line 234)

    do we want the trailing `;`?

src/module/manager.cpp (lines 287 - 294)

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

src/module/manager.cpp (lines 296 - 297)

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

src/module/manager.cpp (line 300)

    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