> On Oct. 1, 2014, 4:35 p.m., Benjamin Hindman wrote: > > include/mesos/module.hpp.in, line 42 > > <https://reviews.apache.org/r/25848/diff/12/?file=709880#file709880line42> > > > > 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.
Tim sugggested that since we only need the version information, including the whole protobuf header chain via mesos.hpp is a bit of an overkill. > On Oct. 1, 2014, 4:35 p.m., Benjamin Hindman wrote: > > include/mesos/module.hpp.in, line 123 > > <https://reviews.apache.org/r/25848/diff/12/?file=709880#file709880line123> > > > > 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. That's an oversight. It should be MESOS_IS_MODULE_COMPATIBILE_FUNCTION. There is a TODO about creating a test that specifically catches this. We'll add the test before landing this patch. > On Oct. 1, 2014, 4:35 p.m., Benjamin Hindman wrote: > > src/module/manager.hpp, line 93 > > <https://reviews.apache.org/r/25848/diff/12/?file=709885#file709885line93> > > > > 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? I guess the term LOAD was supposed to refer to "loading" the module. However, in the current use case, load() return a pointer to the Module object and so I agree that CREATE would be a better term. > On Oct. 1, 2014, 4:35 p.m., Benjamin Hindman wrote: > > include/mesos/module.hpp.in, line 78 > > <https://reviews.apache.org/r/25848/diff/12/?file=709880#file709880line78> > > > > 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. Here are the reasons for favoring the flat symbol layout as opposed to putting it all in a struct: 1. avoid field layout mismatch between the modules and Mesos. 2. avoid versioning of the struct itself (although one can consider using the Module API version for the same purpose). 3. backwards compatibility. If a module was compiled for an older Mesos, it could still be used in the newer Mesos as long as there were no deletions and just additions to the flat symbols. The newer Mesos could check for the presence of a symbol and if not found, take alternate steps. As a side note, a module library may contain multiple module but the Mesos API version, the Mesos release version, and author info fields are constant for the entire module library. The rationale is that the module library can't be compiled against two different APIs or Mesos releases. Thus these values are know at compile time and are constant. Having said that, if we want to use structs, we'll need one for per library library information and another one for per module information. Bernd/Nik, did I miss anything from our earlier discussions? Wild thought: What if we keep the struct, but flatten it as a Json string before passing it on to the Mesos core? This way, we can still handle any addition/deletions to the struct as long as we make sure to not recycle a field name. > On Oct. 1, 2014, 4:35 p.m., Benjamin Hindman wrote: > > src/module/manager.hpp, line 125 > > <https://reviews.apache.org/r/25848/diff/12/?file=709885#file709885line125> > > > > This function is not used. This was a leftover from the isolator module patch. Will remove it from this patch. > On Oct. 1, 2014, 4:35 p.m., Benjamin Hindman wrote: > > src/module/manager.cpp, line 59 > > <https://reviews.apache.org/r/25848/diff/12/?file=709886#file709886line59> > > > > 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? Bernd/Nik: We have been talking about finding a better name anyways. Any ideas? - Kapil ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25848/#review55065 ----------------------------------------------------------- On Oct. 1, 2014, 7:18 p.m., Kapil Arya wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25848/ > ----------------------------------------------------------- > > (Updated Oct. 1, 2014, 7:18 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 e588fd0298f43e44ba01a2b0c907145b801ff1c2 > 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 > >
