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