----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25848/#review55065 -----------------------------------------------------------
include/mesos/module.hpp.in <https://reviews.apache.org/r/25848/#comment95406> Why aren't we just using MESOS_VERSION from mesos/mesos.hpp(.in)? I don't like the idea of introducing another macro that has the same value but giving it a different name. Also, a macro like this really belongs in something like mesos.hpp(.in) since it's generic and useful outside of modules as well. include/mesos/module.hpp.in <https://reviews.apache.org/r/25848/#comment95407> I still am not understanding why we have to introduce more complexity with macros. It seems like we could get away with something far simpler: template <typename T> struct Module { const char* mesos_version; const char* modules_api_version; const char* author_name; const char* author_email; const char* description; // String representation of type T returned from 'create' below. const char* type; // Callback invoked to check version compatibility. bool (*compatible)(); // Callback invoked to create/instantiate an instance of T. T* (*create)(); }; Then a module implementation: Foo* create() { return new FooImpl(); } bool verify() { return true; } extern "C" Module<Foo> example = { MESOS_VERSION, MESOS_MODULES_API_VERSION, "author", "aut...@email.com", "This library provides a Foo implementation", "Foo", compatible, create }; Then loading the module is just loading the library name and this symbol (called 'example') and using that struct to read out the important details, i.e., ignoring errors: Module<Foo>* module = (Module<Foo>*) dlsym(library, "example"); Foo* foo = dynamic_cast<Foo*>(module->create()); foo->function(); Right now there is a lot of "magic" happening through the macros but I'm failing to see the benefit that we really get from this. Seems like we can simplify a lot here which will, IMHO, make creating a module even easier (no need to try and walk through what all the macros are doing), while also making the manager code much easier to read (because we can just do things like 'module->create()' instead of MESOS_GET_MODULE_CREATE_FUNCTION...). In addition, I think the struct approach could also make the module API more extensible, i.e., we can add more fields at the end of the struct as we evolve it in the future. include/mesos/module.hpp.in <https://reviews.apache.org/r/25848/#comment95408> FYI, the macro MESOS_MODULE_COMPATIBILITY_FUNCTION is never defined. I see MESOS_IS_MODULE_COMPATIBILE_FUNCTION defined above, is that what you meant here? This likely tells me that this functionality is not actually tested. Which brings me to a bigger question, what are some concrete examples of when a module is going to want to "have a say" and decided that things are not compatible? I would have assumed at the very least that we'd want to provide some information in the compatibility callback so that a macro had more with which to try and make a call, so I'm having a hard time understanding when just this function getting called on it's own will really be useful. I'm probably just being dense here, so comments, examples would be really helpful. src/module/manager.hpp <https://reviews.apache.org/r/25848/#comment95425> Why call this function 'load' instead of 'create'? Even the macro in the body of this function uses CREATE instead of LOAD. Seems like we're using 'load' for two things, loading the dynamic libraries and creating/instantiating the types, so why not clearly seperate those responsibilities with two different terms? src/module/manager.hpp <https://reviews.apache.org/r/25848/#comment95415> This check is not protected by the mutex! Are you assuming something about the implementation of hashmap::contains that you don't need to protect this? Or the call to 'hashmap::operator []' below? src/module/manager.hpp <https://reviews.apache.org/r/25848/#comment95416> This function is not used. src/module/manager.hpp <https://reviews.apache.org/r/25848/#comment95410> Why introduce the singleton instead of just making all the fields static? Seems like we could simplify this. src/module/manager.hpp <https://reviews.apache.org/r/25848/#comment95414> Why does DynamicLibrary have to be a memory::shared_ptr? From my reading of the code this is never in fact shared. Are you using shared_ptr in order to get automatic object destruction? If so, let's used Owned instead please. src/module/manager.cpp <https://reviews.apache.org/r/25848/#comment95424> Maybe I'm just getting a big grumpy, but I'm really not in favor of overloading the term 'role' here. We've overloaded terms a handful of times in Mesos/libprocess and it has never turned out well. Moreover, by calling this a 'role' we're somehow implying that it's something more than just a type, yet when I look at the possible "roles" suggested here these are just types, Isolator, Authenticator, Allocator, etc. What's the benefit of introducing a new term for this that we need to define for people and they'll have to remember? src/module/manager.cpp <https://reviews.apache.org/r/25848/#comment95420> I understand that we wanted to have some examples, but I'd prefer not to add anything that hasn't yet been modulerized yet (or at least add a comment saying as much). - Benjamin Hindman On Sept. 30, 2014, 11:01 p.m., Kapil Arya wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25848/ > ----------------------------------------------------------- > > (Updated Sept. 30, 2014, 11:01 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 > ----- > > configure.ac 86d448c3ad00ad01d3d069c1039dc7ad524af567 > include/mesos/module.hpp.in PRE-CREATION > src/Makefile.am 27c42dfde45a449750132e416b4eaf776f8c5e3b > src/examples/test_module.hpp PRE-CREATION > src/examples/test_module_impl.cpp PRE-CREATION > src/examples/test_module_impl2.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 > >
