> On Oct. 1, 2014, 1: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. > > Kapil Arya wrote: > 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.
@Ben "far simpler" depends on your angle. We used the macros to have as little structure and protocol for module *writers* as possible. This means more complexity in the module system, but that's IMHO still a valid tradeoff choice one can make. Your solution optimizes for less complexity in the module system - at the cost of added code and rules for every module implementation. But since the difference is not huge and we are going to have pretty finite numbers of modules, we can drop the design goal we had and go with your approach. I do not like macros either. We will lose a little bit of potential backward compatibility with the struct approach, but I think here we also went too far trying to maintain it. Relying on ordered struct extension should suffice in that regard. And as Kapil wrote in item 2 we can always bump the API version if necessary. The struct approach comes without library declaration. Simplest solution: just ignore this and verify each module independently. - Bernd ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25848/#review55065 ----------------------------------------------------------- On Oct. 1, 2014, 4: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, 4: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 > >
