----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25848/#review55487 -----------------------------------------------------------
include/mesos/module.hpp <https://reviews.apache.org/r/25848/#comment95861> Aren't we renaming role->kind? include/mesos/module.hpp <https://reviews.apache.org/r/25848/#comment95862> Need to update this text. include/mesos/module.hpp <https://reviews.apache.org/r/25848/#comment95863> I'd put this first to emphasize that it changes less often. include/mesos/module.hpp <https://reviews.apache.org/r/25848/#comment95864> I'd put this first. include/mesos/module.hpp <https://reviews.apache.org/r/25848/#comment95865> I was expecting a "T create();" method declaration here. include/mesos/module.hpp <https://reviews.apache.org/r/25848/#comment95866> Shouldn't this be in a different file? include/mesos/module.hpp <https://reviews.apache.org/r/25848/#comment95867> I don't think we want all module kinds in one place. Then someone who writes a module of kind A needs to import the declaration for kind B and C and so on as well. Why not put the declaration of the Isolator module into whatever file declares Isolator? src/examples/example_module_impl.cpp <https://reviews.apache.org/r/25848/#comment95868> On principle, I prefer avoiding forward declarations. (Extra maintenance cost, extra code reading effort.) src/examples/test_module.hpp <https://reviews.apache.org/r/25848/#comment95869> Now there is a confusion whether TestModule is "the module" or exampleModule is "the module". I suggest we only call one of the two "the module". Since "Isolator", etc. aren't called module (which is good, as their module-ness should not matter at their use sites), I would opt for renaming TestModule. Suggestion: TestCalculator. src/examples/test_module_impl.cpp <https://reviews.apache.org/r/25848/#comment95870> Please no forward decls. src/examples/test_module_impl.cpp <https://reviews.apache.org/r/25848/#comment95871> A little motivation for this file in a comment somewhere in it would be good. Apparently this is used to spoof deviating Mesos and API versions. But I don't want to read all the code to come to this conclusion. src/examples/test_module_impl.cpp <https://reviews.apache.org/r/25848/#comment95875> We could just place NULL into the ModuleBase struct here, to test yet another code branch in the module manager. src/module/manager.hpp <https://reviews.apache.org/r/25848/#comment95872> Suggestion: LibraryModulesMap Note the extra "s". The plural iundicates that each library can have multiple modules. src/module/manager.cpp <https://reviews.apache.org/r/25848/#comment95873> Naked string? src/module/manager.cpp <https://reviews.apache.org/r/25848/#comment95876> Maybe add a comment here? // If the flag value is not a JSON array, it is regarded as a file path. src/module/manager.cpp <https://reviews.apache.org/r/25848/#comment95877> Can we use a dynamic_cast here for added safety? This means that ModuleBase becomes an abstract class with a virtual method, though. We could make "compatible" that method. Instead of a bool it would return an Option<bool>. A "none" result would then take over the role of the methid being NULL. - Bernd Mathiske On Oct. 3, 2014, 4:46 p.m., Kapil Arya wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25848/ > ----------------------------------------------------------- > > (Updated Oct. 3, 2014, 4:46 p.m.) > > > Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, > and Timothy St. Clair. > > > Bugs: MESOS-1384 > https://issues.apache.org/jira/browse/MESOS-1384 > > > Repository: mesos-git > > > Description > ------- > > Adding a first class primitive, abstraction and process for dynamic library > writing and loading can make it easier to extend inner workings of Mesos. > Making it possible to have dynamic loadable resource allocators, isolators, > containerizes, authenticators and much more. > > > Diffs > ----- > > include/mesos/module.hpp PRE-CREATION > src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724 > src/examples/example_module_impl.cpp PRE-CREATION > src/examples/test_module.hpp PRE-CREATION > src/examples/test_module_impl.cpp PRE-CREATION > src/module/manager.hpp PRE-CREATION > src/module/manager.cpp PRE-CREATION > src/tests/module_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/25848/diff/ > > > Testing > ------- > > Ran make check with added tests for verifying library/module loading and > simple version check. > > > Thanks, > > Kapil Arya > >
