mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Kapil Arya" <ka...@mesosphere.io>
Subject Re: Review Request 25848: Introducing mesos modules.
Date Wed, 01 Oct 2014 23:18:32 GMT


> 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",
> >       "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.

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
> 
>


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