mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bernd Mathiske" <be...@mesosphere.io>
Subject Re: Review Request 25848: Introducing mesos modules.
Date Thu, 02 Oct 2014 10:49:31 GMT


> 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",
> >       "author@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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message