----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25848/#review54043 -----------------------------------------------------------
First pass src/examples/test_module.cpp <https://reviews.apache.org/r/25848/#comment93939> The build bot failed this include - mind take a look? src/examples/test_module.cpp <https://reviews.apache.org/r/25848/#comment93941> This file will probably serve as a template for new templates, so let's add some comments on what the MESOS_MODULE_LIBRARY() and MESOS_MODULE() macros do. src/examples/test_module.cpp <https://reviews.apache.org/r/25848/#comment93940> Mind throwing in a comment on what/where the example module is used? src/examples/test_module.cpp <https://reviews.apache.org/r/25848/#comment93975> You could also throw in a comment here on what function declaration that gets generated i.e. exported symbol name and return type. src/module/manager.hpp <https://reviews.apache.org/r/25848/#comment93942> Mind mentioning where you get this configuration from i.e. "--modules" src/module/manager.hpp <https://reviews.apache.org/r/25848/#comment93943> { on it's own line src/module/manager.hpp <https://reviews.apache.org/r/25848/#comment93967> End comment with period src/module/manager.hpp <https://reviews.apache.org/r/25848/#comment93966> Isn't "module" implied by the module manager? How about ->load() and ->contains() ? src/module/manager.hpp <https://reviews.apache.org/r/25848/#comment93969> Can we find some ABI documentation that supports this claim and maybe throw in a reference? src/module/manager.hpp <https://reviews.apache.org/r/25848/#comment93944> { on it's own line src/module/manager.hpp <https://reviews.apache.org/r/25848/#comment93945> Newline between loadModuleLibrary and verify module role. Also, isn't 'module' implied? loadLibrary / verifyRole would be a bit more concise. src/module/manager.hpp <https://reviews.apache.org/r/25848/#comment93946> You can use 'CHECK_NOTNULL(lib)->' to guard the libary pointer. src/module/manager.hpp <https://reviews.apache.org/r/25848/#comment93970> Access to those need to be guarded, right? Or do you explicitally call out that the module manager isn't thread safe? src/module/manager.hpp <https://reviews.apache.org/r/25848/#comment93964> How about using shared_ptr's instead? If not, how is cleaning up all the allocated libraries? src/module/manager.cpp <https://reviews.apache.org/r/25848/#comment93971> newline after 'version.hpp' src/module/manager.cpp <https://reviews.apache.org/r/25848/#comment93963> Ben, we did the module manager as a singleton. I know it is a uncommon pattern in Mesos in general. Do you have any thoughts on this? We will need to take care of thread-safety with whatever state we maintain in the module manager. src/module/manager.cpp <https://reviews.apache.org/r/25848/#comment93961> How about using shared_ptr instead of a raw pointer? src/module/manager.cpp <https://reviews.apache.org/r/25848/#comment93962> Mind throwing this on a new line and indent 4 spaces? Here and below src/module/manager.cpp <https://reviews.apache.org/r/25848/#comment93978> This block is very dense - mind spreading it out a little bit? src/module/manager.cpp <https://reviews.apache.org/r/25848/#comment93957> Odd indentation here. src/slave/flags.hpp <https://reviews.apache.org/r/25848/#comment93948> Let's tie the modules in after the core patch has landed. Also, we need to it for both masters and slaves (alongside starting to wire up module loading for isolators, authenticators, ...) TL;DR Let's review the src/slave/flags.hpp and src/slave/main.cpp changes (with a master equivalent) in a dependent patch. In the new review, please expand the help text and throw in some examples. You can put in the expected format as you listed in the module manager header. src/slave/main.cpp <https://reviews.apache.org/r/25848/#comment93956> See comment above. src/tests/module.hpp <https://reviews.apache.org/r/25848/#comment93949> Mind throwing in a comment on where this is implemented / how it is used? src/tests/module.hpp <https://reviews.apache.org/r/25848/#comment93950> And perhaps highlight the virtual destructor's importance (in order to clean up the object in the library's context). People are probably going to use this as a template for writing new modules :-) src/tests/module_tests.cpp <https://reviews.apache.org/r/25848/#comment93947> This seems to be repeated a couple of times - can we centralize in a local helper? src/tests/module_tests.cpp <https://reviews.apache.org/r/25848/#comment93955> When are you cleaning up this file? Use mktemp instead and kill the file when you are done :-) src/tests/module_tests.cpp <https://reviews.apache.org/r/25848/#comment93951> Mind throwing in a comment on why we expect exactly those libraries? src/tests/module_tests.cpp <https://reviews.apache.org/r/25848/#comment93953> Two new lines src/tests/module_tests.cpp <https://reviews.apache.org/r/25848/#comment93952> Where are we going to capture the negative tests? Wrong library versions, trying to load non-mesos modules libraries (libc.so for example)? - Niklas Nielsen On Sept. 19, 2014, 1:40 p.m., Kapil Arya wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25848/ > ----------------------------------------------------------- > > (Updated Sept. 19, 2014, 1:40 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 9b973e5503e30180045e270220987ba647da8038 > src/examples/test_module.cpp PRE-CREATION > src/module/manager.hpp PRE-CREATION > src/module/manager.cpp PRE-CREATION > src/slave/flags.hpp 21e00212bc402674eaea73b44b3f91df477a7213 > src/slave/main.cpp 2c4d365a04acbcb382e978d811a318130484b3d5 > src/tests/module.hpp 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 > >
