mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benjamin Hindman" <b...@berkeley.edu>
Subject Re: Review Request 25848: Introducing mesos modules.
Date Wed, 01 Oct 2014 20:35:50 GMT

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



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


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