You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Kapil Arya <ka...@mesosphere.io> on 2014/10/01 01:01:02 UTC
Re: Review Request 25848: Introducing mesos modules.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/
-----------------------------------------------------------
(Updated Sept. 30, 2014, 7:01 p.m.)
Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, and Timothy St. Clair.
Changes
-------
Addressed most of Nik's comments.
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 (updated)
-----
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
Re: Review Request 25848: Introducing mesos modules.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review55886
-----------------------------------------------------------
Ship it!
Alright! Kapil and I have made a few minor style/syntax changes together and this is looking good to go! Will commit now! ;-)
- Benjamin Hindman
On Oct. 8, 2014, 8:52 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2014, 8:52 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
> -----
>
> include/mesos/module.hpp PRE-CREATION
> src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724
> src/examples/example_module_impl.cpp PRE-CREATION
> src/examples/test_module.hpp PRE-CREATION
> src/messages/messages.proto 9ff06b38086010df362036c695a5222371f70f4d
> src/module/manager.hpp PRE-CREATION
> src/module/manager.cpp PRE-CREATION
> src/tests/master_tests.cpp 705e5f2c0f9d693946e722cef63f38f3bff0d46b
> 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
>
>
Re: Review Request 25848: Introducing mesos modules.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/
-----------------------------------------------------------
(Updated Oct. 8, 2014, 4:52 p.m.)
Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, and Timothy St. Clair.
Changes
-------
Rename ModuleManager::shutdown() -> ModuleManager::unloadAll().
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 (updated)
-----
include/mesos/module.hpp PRE-CREATION
src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724
src/examples/example_module_impl.cpp PRE-CREATION
src/examples/test_module.hpp PRE-CREATION
src/messages/messages.proto 9ff06b38086010df362036c695a5222371f70f4d
src/module/manager.hpp PRE-CREATION
src/module/manager.cpp PRE-CREATION
src/tests/master_tests.cpp 705e5f2c0f9d693946e722cef63f38f3bff0d46b
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
Re: Review Request 25848: Introducing mesos modules.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review55874
-----------------------------------------------------------
src/module/manager.cpp
<https://reviews.apache.org/r/25848/#comment96244>
Just noticed this - shutdown() doesn't describe what you expect the function to do or how you use it. How about ::clear() or ::unloadAll()?
- Niklas Nielsen
On Oct. 8, 2014, 9:22 a.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2014, 9:22 a.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
> -----
>
> include/mesos/module.hpp PRE-CREATION
> src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724
> src/examples/example_module_impl.cpp PRE-CREATION
> src/examples/test_module.hpp PRE-CREATION
> src/messages/messages.proto 9ff06b38086010df362036c695a5222371f70f4d
> src/module/manager.hpp PRE-CREATION
> src/module/manager.cpp PRE-CREATION
> src/tests/master_tests.cpp 705e5f2c0f9d693946e722cef63f38f3bff0d46b
> 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
>
>
Re: Review Request 25848: Introducing mesos modules.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review55823
-----------------------------------------------------------
Ship it!
Ship It!
- Niklas Nielsen
On Oct. 8, 2014, 9:22 a.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2014, 9:22 a.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
> -----
>
> include/mesos/module.hpp PRE-CREATION
> src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724
> src/examples/example_module_impl.cpp PRE-CREATION
> src/examples/test_module.hpp PRE-CREATION
> src/messages/messages.proto 9ff06b38086010df362036c695a5222371f70f4d
> src/module/manager.hpp PRE-CREATION
> src/module/manager.cpp PRE-CREATION
> src/tests/master_tests.cpp 705e5f2c0f9d693946e722cef63f38f3bff0d46b
> 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
>
>
Re: Review Request 25848: Introducing mesos modules.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/
-----------------------------------------------------------
(Updated Oct. 8, 2014, 12:22 p.m.)
Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, and Timothy St. Clair.
Changes
-------
Make return type of MM::load() void since it always succeeds; Replace CHECK with Error in MM::create();
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 (updated)
-----
include/mesos/module.hpp PRE-CREATION
src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724
src/examples/example_module_impl.cpp PRE-CREATION
src/examples/test_module.hpp PRE-CREATION
src/messages/messages.proto 9ff06b38086010df362036c695a5222371f70f4d
src/module/manager.hpp PRE-CREATION
src/module/manager.cpp PRE-CREATION
src/tests/master_tests.cpp 705e5f2c0f9d693946e722cef63f38f3bff0d46b
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
Re: Review Request 25848: Introducing mesos modules.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/
-----------------------------------------------------------
(Updated Oct. 8, 2014, 2:31 a.m.)
Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, and Timothy St. Clair.
Changes
-------
Bug fixes to module version checking and testing; added ModuleManager::shutdown() to reinitialize the data structures (for make check only).
>From a given list of modules, if some modules fail verfication checks, etc., load the remaining (verified) modules regardless.
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 (updated)
-----
include/mesos/module.hpp PRE-CREATION
src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724
src/examples/example_module_impl.cpp PRE-CREATION
src/examples/test_module.hpp PRE-CREATION
src/messages/messages.proto 9ff06b38086010df362036c695a5222371f70f4d
src/module/manager.hpp PRE-CREATION
src/module/manager.cpp PRE-CREATION
src/tests/master_tests.cpp 705e5f2c0f9d693946e722cef63f38f3bff0d46b
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
Re: Review Request 25848: Introducing mesos modules.
Posted by Michael Park <mc...@gmail.com>.
> On Oct. 7, 2014, 11:43 p.m., Michael Park wrote:
> > src/module/manager.hpp, line 75
> > <https://reviews.apache.org/r/25848/diff/17/?file=714903#file714903line75>
> >
> > Is the result of `module->create()` supposed to be a singleton? As in, if we call `create` twice with the same module name, should we end up with a pointer to a singleton? or is it supposed to return a pointer to a new instance?
>
> Kapil Arya wrote:
> Mesos is not imposing any restriction and it is upto the module writer to decide. AFAIK, for our current use cases, we won't be calling create() twice on the same module. However, one can imagine use cases where multiple modules are created by passing different arguments to create().
Ok, so it's up to the module writer to make their `create` function return either a pointer to a singleton or have it construct a new one. Then would it maybe make sense for the variable to be called something other than `singleton`? Maybe `kind` or `instance`?
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review55732
-----------------------------------------------------------
On Oct. 7, 2014, 11:07 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2014, 11:07 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
> -----
>
> include/mesos/module.hpp PRE-CREATION
> src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724
> src/examples/example_module_impl.cpp PRE-CREATION
> src/examples/test_module.hpp PRE-CREATION
> src/messages/messages.proto 9ff06b38086010df362036c695a5222371f70f4d
> src/module/manager.hpp PRE-CREATION
> src/module/manager.cpp PRE-CREATION
> src/tests/master_tests.cpp 705e5f2c0f9d693946e722cef63f38f3bff0d46b
> 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
>
>
Re: Review Request 25848: Introducing mesos modules.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On Oct. 7, 2014, 7:43 p.m., Michael Park wrote:
> > src/module/manager.hpp, line 75
> > <https://reviews.apache.org/r/25848/diff/17/?file=714903#file714903line75>
> >
> > Is the result of `module->create()` supposed to be a singleton? As in, if we call `create` twice with the same module name, should we end up with a pointer to a singleton? or is it supposed to return a pointer to a new instance?
Mesos is not imposing any restriction and it is upto the module writer to decide. AFAIK, for our current use cases, we won't be calling create() twice on the same module. However, one can imagine use cases where multiple modules are created by passing different arguments to create().
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review55732
-----------------------------------------------------------
On Oct. 7, 2014, 7:07 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2014, 7:07 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
> -----
>
> include/mesos/module.hpp PRE-CREATION
> src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724
> src/examples/example_module_impl.cpp PRE-CREATION
> src/examples/test_module.hpp PRE-CREATION
> src/messages/messages.proto 9ff06b38086010df362036c695a5222371f70f4d
> src/module/manager.hpp PRE-CREATION
> src/module/manager.cpp PRE-CREATION
> src/tests/master_tests.cpp 705e5f2c0f9d693946e722cef63f38f3bff0d46b
> 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
>
>
Re: Review Request 25848: Introducing mesos modules.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review55732
-----------------------------------------------------------
src/module/manager.hpp
<https://reviews.apache.org/r/25848/#comment96122>
Is the result of `module->create()` supposed to be a singleton? As in, if we call `create` twice with the same module name, should we end up with a pointer to a singleton? or is it supposed to return a pointer to a new instance?
- Michael Park
On Oct. 7, 2014, 11:07 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2014, 11:07 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
> -----
>
> include/mesos/module.hpp PRE-CREATION
> src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724
> src/examples/example_module_impl.cpp PRE-CREATION
> src/examples/test_module.hpp PRE-CREATION
> src/messages/messages.proto 9ff06b38086010df362036c695a5222371f70f4d
> src/module/manager.hpp PRE-CREATION
> src/module/manager.cpp PRE-CREATION
> src/tests/master_tests.cpp 705e5f2c0f9d693946e722cef63f38f3bff0d46b
> 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
>
>
Re: Review Request 25848: Introducing mesos modules.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Oct. 8, 2014, 1:17 a.m., Michael Park wrote:
> > src/module/manager.hpp, lines 95-105
> > <https://reviews.apache.org/r/25848/diff/17/?file=714903#file714903line95>
> >
> > I would suggest changing these to be static functions that return static singletons as per [MESOS-1023](https://issues.apache.org/jira/browse/MESOS-1023). Something like,
> >
> > ```cpp
> > static pthread_mutex_t &mutex() {
> > static pthread_mutex_t singleton = PTHREAD_MUTEX_INITIALIZER;
> > return singleton;
> > }
> > ```
Maybe I'm missing something here, but how does this help? The destructors will still get called for static variables in functions (even static functions).
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review55735
-----------------------------------------------------------
On Oct. 7, 2014, 11:07 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2014, 11:07 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
> -----
>
> include/mesos/module.hpp PRE-CREATION
> src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724
> src/examples/example_module_impl.cpp PRE-CREATION
> src/examples/test_module.hpp PRE-CREATION
> src/messages/messages.proto 9ff06b38086010df362036c695a5222371f70f4d
> src/module/manager.hpp PRE-CREATION
> src/module/manager.cpp PRE-CREATION
> src/tests/master_tests.cpp 705e5f2c0f9d693946e722cef63f38f3bff0d46b
> 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
>
>
Re: Review Request 25848: Introducing mesos modules.
Posted by Michael Park <mc...@gmail.com>.
> On Oct. 8, 2014, 1:17 a.m., Michael Park wrote:
> > src/module/manager.hpp, lines 95-105
> > <https://reviews.apache.org/r/25848/diff/17/?file=714903#file714903line95>
> >
> > I would suggest changing these to be static functions that return static singletons as per [MESOS-1023](https://issues.apache.org/jira/browse/MESOS-1023). Something like,
> >
> > ```cpp
> > static pthread_mutex_t &mutex() {
> > static pthread_mutex_t singleton = PTHREAD_MUTEX_INITIALIZER;
> > return singleton;
> > }
> > ```
>
> Benjamin Hindman wrote:
> Maybe I'm missing something here, but how does this help? The destructors will still get called for static variables in functions (even static functions).
The difference is that initialization order of global/static member variables across translation units are non-determistic. In this case, the variables aren't accessible from another translation unit, but we might make one of them `public` in the future.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review55735
-----------------------------------------------------------
On Oct. 8, 2014, 6:31 a.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2014, 6:31 a.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
> -----
>
> include/mesos/module.hpp PRE-CREATION
> src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724
> src/examples/example_module_impl.cpp PRE-CREATION
> src/examples/test_module.hpp PRE-CREATION
> src/messages/messages.proto 9ff06b38086010df362036c695a5222371f70f4d
> src/module/manager.hpp PRE-CREATION
> src/module/manager.cpp PRE-CREATION
> src/tests/master_tests.cpp 705e5f2c0f9d693946e722cef63f38f3bff0d46b
> 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
>
>
Re: Review Request 25848: Introducing mesos modules.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review55735
-----------------------------------------------------------
src/module/manager.hpp
<https://reviews.apache.org/r/25848/#comment96129>
I would suggest changing these to be static functions that return static singletons as per [MESOS-1023](https://issues.apache.org/jira/browse/MESOS-1023). Something like,
```cpp
static pthread_mutex_t &mutex() {
static pthread_mutex_t singleton = PTHREAD_MUTEX_INITIALIZER;
return singleton;
}
```
- Michael Park
On Oct. 7, 2014, 11:07 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2014, 11:07 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
> -----
>
> include/mesos/module.hpp PRE-CREATION
> src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724
> src/examples/example_module_impl.cpp PRE-CREATION
> src/examples/test_module.hpp PRE-CREATION
> src/messages/messages.proto 9ff06b38086010df362036c695a5222371f70f4d
> src/module/manager.hpp PRE-CREATION
> src/module/manager.cpp PRE-CREATION
> src/tests/master_tests.cpp 705e5f2c0f9d693946e722cef63f38f3bff0d46b
> 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
>
>
Re: Review Request 25848: Introducing mesos modules.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/
-----------------------------------------------------------
(Updated Oct. 7, 2014, 7:07 p.m.)
Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, and Timothy St. Clair.
Changes
-------
Reverted changes to common/parse.hpp; will add back later when we enable master/slave flags.
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 (updated)
-----
include/mesos/module.hpp PRE-CREATION
src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724
src/examples/example_module_impl.cpp PRE-CREATION
src/examples/test_module.hpp PRE-CREATION
src/messages/messages.proto 9ff06b38086010df362036c695a5222371f70f4d
src/module/manager.hpp PRE-CREATION
src/module/manager.cpp PRE-CREATION
src/tests/master_tests.cpp 705e5f2c0f9d693946e722cef63f38f3bff0d46b
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
Re: Review Request 25848: Introducing mesos modules.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/
-----------------------------------------------------------
(Updated Oct. 7, 2014, 6:59 p.m.)
Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, and Timothy St. Clair.
Changes
-------
Fixed some typos.
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 (updated)
-----
include/mesos/module.hpp PRE-CREATION
src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724
src/common/parse.hpp e6153d8a1f25bc9ddbe1e391306beeacfc8d5ff6
src/examples/example_module_impl.cpp PRE-CREATION
src/examples/test_module.hpp PRE-CREATION
src/messages/messages.proto 9ff06b38086010df362036c695a5222371f70f4d
src/module/manager.hpp PRE-CREATION
src/module/manager.cpp PRE-CREATION
src/tests/master_tests.cpp 705e5f2c0f9d693946e722cef63f38f3bff0d46b
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
Re: Review Request 25848: Introducing mesos modules.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/
-----------------------------------------------------------
(Updated Oct. 7, 2014, 6:43 p.m.)
Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, and Timothy St. Clair.
Changes
-------
Addressed most of BenH's and Bernd's comments; use protobufs for Module lists.
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 (updated)
-----
include/mesos/module.hpp PRE-CREATION
src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724
src/common/parse.hpp e6153d8a1f25bc9ddbe1e391306beeacfc8d5ff6
src/examples/example_module_impl.cpp PRE-CREATION
src/examples/test_module.hpp PRE-CREATION
src/messages/messages.proto 9ff06b38086010df362036c695a5222371f70f4d
src/module/manager.hpp PRE-CREATION
src/module/manager.cpp PRE-CREATION
src/tests/master_tests.cpp 705e5f2c0f9d693946e722cef63f38f3bff0d46b
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
Re: Review Request 25848: Introducing mesos modules.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On Oct. 6, 2014, 11:56 p.m., Benjamin Hindman wrote:
> > src/examples/example_module_impl.cpp, line 32
> > <https://reviews.apache.org/r/25848/diff/14/?file=713786#file713786line32>
> >
> > To Bernd's point above, if you declare/define this at the bottom then you won't need the forward declarations up top. I like this pattern, define everything you need before the Module<T> declaration/definition, which will tend to come at the bottom of files!
> >
> > A bigger concern I have, however, is regarding the lifetime of these objects. What happens if/when the dynamic library gets closed? Or what about when the application gets shutdown? Are these subject to destructors/cleanup? If so, either we need to have module writers create Module<T>* or we need to do deep copies when we read the symbols the first time (which actually still won't be immune to potential destruction because an application might be shutting down right when we start to read the symbol which could cause a crash, probably not the worst thing since the application was already being shutdown but still not a great user experience).
After giving it some more thought, I realized that neither the deep copy, nor putting the object on heap help us with improper interleaving during application shutdown. In both cases, the create() and compatible() methods will still be part of the dynamic library and if it is dlclose()'d, then the application is going to crash if someone tries to call create() or compatible.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review55568
-----------------------------------------------------------
On Oct. 7, 2014, 6:43 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2014, 6:43 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
> -----
>
> include/mesos/module.hpp PRE-CREATION
> src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724
> src/common/parse.hpp e6153d8a1f25bc9ddbe1e391306beeacfc8d5ff6
> src/examples/example_module_impl.cpp PRE-CREATION
> src/examples/test_module.hpp PRE-CREATION
> src/messages/messages.proto 9ff06b38086010df362036c695a5222371f70f4d
> src/module/manager.hpp PRE-CREATION
> src/module/manager.cpp PRE-CREATION
> src/tests/master_tests.cpp 705e5f2c0f9d693946e722cef63f38f3bff0d46b
> 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
>
>
Re: Review Request 25848: Introducing mesos modules.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review55568
-----------------------------------------------------------
Great iteration Kapil! This is your first major contribution to the code base so there are some other helpful things to point out (like our use of JSON to protobuf, see below), so after this next iteration I think we'll be very near to commit!
src/examples/example_module_impl.cpp
<https://reviews.apache.org/r/25848/#comment95926>
To Bernd's point above, if you declare/define this at the bottom then you won't need the forward declarations up top. I like this pattern, define everything you need before the Module<T> declaration/definition, which will tend to come at the bottom of files!
A bigger concern I have, however, is regarding the lifetime of these objects. What happens if/when the dynamic library gets closed? Or what about when the application gets shutdown? Are these subject to destructors/cleanup? If so, either we need to have module writers create Module<T>* or we need to do deep copies when we read the symbols the first time (which actually still won't be immune to potential destruction because an application might be shutting down right when we start to read the symbol which could cause a crash, probably not the worst thing since the application was already being shutdown but still not a great user experience).
src/module/manager.hpp
<https://reviews.apache.org/r/25848/#comment95999>
Please put * next to type name, thanks!
src/module/manager.hpp
<https://reviews.apache.org/r/25848/#comment95939>
One of the main reasons we use JSON is so that we can easily convert to and from typed data structures represented by protobufs in our C++ code. So I'd like to see a protobuf here. In general it makes working with JSON much cleaner (and we don't have to have wierd typedefs). Based on your current JSON I could see a protobuf looking something like (but if this doesn't match how you'd want to use it, then lets change the JSON!):
message Modules {
message Library {
optional string path = 1;
repeated string modules = 2;
}
repeated Library libraries = 1;
}
Then we'd add a 'parse' function in src/common/parse.hpp which is where the other generic flag parsing functions reside. Then ModulesManager::load should just take an instance of Modules, without needing to first parse a flag (the Flags infrastructure will automagically take care of calling the correct parse function to get an instance of Modules). Leads to a clean separation of concerns! And now we have a datatype that we can do things with cleanly in C++, like work with in the tests (where I've made some comments as well).
src/module/manager.hpp
<https://reviews.apache.org/r/25848/#comment95998>
Use const string& and * on type name please, thanks!
src/module/manager.hpp
<https://reviews.apache.org/r/25848/#comment95994>
To the best of my knowledge this is only being used to make sure that we keep our instance of DynamicLibrary from getting destructed (and thus closing the library). Maybe the idea was to at some point not reopen a dynamic library that had already been opened? But that doesn't appear to be happening now.
At the very least we should document this, but I could also see us changing this around to be a hashmap<ModuleBase*, Shared<DynamicLibrary>> where each instance of ModuleBase that depends on that DynamicLibrary has an entry since this seems slightly less arbitrary than saving the "path" of the DynamicLibrary (which might not even be a path IIUC). Alternatively, maybe we skip the map all together here and just go with a data structure that stores all the DynamicLibrary instances (i.e., using a std::list?) and add a TODO that says make them addressable only if/when we decide to implement something that let's you remove a module (and thus close it's dynamic library). But heck, I'd even be okay with just putting the DynamicLibrary on the heap in 'load' without an Owned/Shared and putting a TODO and comment directly there explaining that currently we never delete the DynamicLibrary instance nor do we expose a way to delete it so for now we just let the pointer dangle!
src/module/manager.hpp
<https://reviews.apache.org/r/25848/#comment95995>
Can we add a brief comment here that this is from a "module name" to the actual ModuleBase and that if two modules from different libraries have the same name then the last one specified in the protobuf Modules will be picked. This needs to clearly be documented for the modules writer as well, and is what will probably lead us into writing module names that are of the format org_apache_mesos_test. In fact, if this is our plan I wouldn't be opposed to starting this convention with our example modules too to set the precedent.
src/module/manager.cpp
<https://reviews.apache.org/r/25848/#comment95941>
This seems unnecessarily severe. If someone makes a mistake constructing their module they've just caused the master/slave to crash. Why not return Error if these are NULL instead?
src/module/manager.cpp
<https://reviews.apache.org/r/25848/#comment95993>
I've seen this a couple of times throughout this review, please use const & for the "left hand side" of the foreach* so that we can avoid a copy. Give a quick skim through the rest of this review for other places please, thanks!
src/tests/module_tests.cpp
<https://reviews.apache.org/r/25848/#comment95996>
const &
src/tests/module_tests.cpp
<https://reviews.apache.org/r/25848/#comment95942>
We can use the Modules object defined above rather than doing JSON construction ourself (which is usually code smell to me).
Modules modules;
Modules::Library* library = modules.add_libraries();
library->set_path(getLibraryPath("examplemodule"));
library->add_modules("examplemodule");
... JSON::Protobuf(modules) ...
src/tests/module_tests.cpp
<https://reviews.apache.org/r/25848/#comment95934>
I see that you can't pass DynamicLibrary as a const& because loadSymbol is not const, but it really should be. We avoid passing non-const references where possible, so please pass a pointer here instead. Thanks!
src/tests/module_tests.cpp
<https://reviews.apache.org/r/25848/#comment95933>
Why do you need functions for this? Why not simplify and just load the symbol you want to change and change it directly? Same applies below. I see Bernd commented that the use of these functions was hard to track down, so why not eliminate that function all together! ;-)
- Benjamin Hindman
On Oct. 3, 2014, 11:46 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2014, 11:46 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
> -----
>
> include/mesos/module.hpp PRE-CREATION
> src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724
> src/examples/example_module_impl.cpp PRE-CREATION
> src/examples/test_module.hpp PRE-CREATION
> src/examples/test_module_impl.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
>
>
Re: Review Request 25848: Introducing mesos modules.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On Oct. 6, 2014, 7:25 a.m., Bernd Mathiske wrote:
> > src/examples/test_module.hpp, line 26
> > <https://reviews.apache.org/r/25848/diff/14/?file=713787#file713787line26>
> >
> > Now there is a confusion whether TestModule is "the module" or exampleModule is "the module". I suggest we only call one of the two "the module". Since "Isolator", etc. aren't called module (which is good, as their module-ness should not matter at their use sites), I would opt for renaming TestModule. Suggestion: TestCalculator.
"TestModule" is along the same lines as TestFramework, TestExecutor, etc. Is there a better name then "exampleModule"?
> On Oct. 6, 2014, 7:25 a.m., Bernd Mathiske wrote:
> > src/module/manager.cpp, line 229
> > <https://reviews.apache.org/r/25848/diff/14/?file=713790#file713790line229>
> >
> > Can we use a dynamic_cast here for added safety? This means that ModuleBase becomes an abstract class with a virtual method, though. We could make "compatible" that method. Instead of a bool it would return an Option<bool>. A "none" result would then take over the role of the methid being NULL.
In ModuleBase, "compatible" is merely a function pointer, not a function and so it doesn't make sense to include a virtual method here.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review55487
-----------------------------------------------------------
On Oct. 7, 2014, 6:43 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2014, 6:43 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
> -----
>
> include/mesos/module.hpp PRE-CREATION
> src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724
> src/common/parse.hpp e6153d8a1f25bc9ddbe1e391306beeacfc8d5ff6
> src/examples/example_module_impl.cpp PRE-CREATION
> src/examples/test_module.hpp PRE-CREATION
> src/messages/messages.proto 9ff06b38086010df362036c695a5222371f70f4d
> src/module/manager.hpp PRE-CREATION
> src/module/manager.cpp PRE-CREATION
> src/tests/master_tests.cpp 705e5f2c0f9d693946e722cef63f38f3bff0d46b
> 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
>
>
Re: Review Request 25848: Introducing mesos modules.
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review55487
-----------------------------------------------------------
include/mesos/module.hpp
<https://reviews.apache.org/r/25848/#comment95861>
Aren't we renaming role->kind?
include/mesos/module.hpp
<https://reviews.apache.org/r/25848/#comment95862>
Need to update this text.
include/mesos/module.hpp
<https://reviews.apache.org/r/25848/#comment95863>
I'd put this first to emphasize that it changes less often.
include/mesos/module.hpp
<https://reviews.apache.org/r/25848/#comment95864>
I'd put this first.
include/mesos/module.hpp
<https://reviews.apache.org/r/25848/#comment95865>
I was expecting a "T create();" method declaration here.
include/mesos/module.hpp
<https://reviews.apache.org/r/25848/#comment95866>
Shouldn't this be in a different file?
include/mesos/module.hpp
<https://reviews.apache.org/r/25848/#comment95867>
I don't think we want all module kinds in one place. Then someone who writes a module of kind A needs to import the declaration for kind B and C and so on as well.
Why not put the declaration of the Isolator module into whatever file declares Isolator?
src/examples/example_module_impl.cpp
<https://reviews.apache.org/r/25848/#comment95868>
On principle, I prefer avoiding forward declarations. (Extra maintenance cost, extra code reading effort.)
src/examples/test_module.hpp
<https://reviews.apache.org/r/25848/#comment95869>
Now there is a confusion whether TestModule is "the module" or exampleModule is "the module". I suggest we only call one of the two "the module". Since "Isolator", etc. aren't called module (which is good, as their module-ness should not matter at their use sites), I would opt for renaming TestModule. Suggestion: TestCalculator.
src/examples/test_module_impl.cpp
<https://reviews.apache.org/r/25848/#comment95870>
Please no forward decls.
src/examples/test_module_impl.cpp
<https://reviews.apache.org/r/25848/#comment95871>
A little motivation for this file in a comment somewhere in it would be good. Apparently this is used to spoof deviating Mesos and API versions. But I don't want to read all the code to come to this conclusion.
src/examples/test_module_impl.cpp
<https://reviews.apache.org/r/25848/#comment95875>
We could just place NULL into the ModuleBase struct here, to test yet another code branch in the module manager.
src/module/manager.hpp
<https://reviews.apache.org/r/25848/#comment95872>
Suggestion: LibraryModulesMap
Note the extra "s".
The plural iundicates that each library can have multiple modules.
src/module/manager.cpp
<https://reviews.apache.org/r/25848/#comment95873>
Naked string?
src/module/manager.cpp
<https://reviews.apache.org/r/25848/#comment95876>
Maybe add a comment here?
// If the flag value is not a JSON array, it is regarded as a file path.
src/module/manager.cpp
<https://reviews.apache.org/r/25848/#comment95877>
Can we use a dynamic_cast here for added safety? This means that ModuleBase becomes an abstract class with a virtual method, though. We could make "compatible" that method. Instead of a bool it would return an Option<bool>. A "none" result would then take over the role of the methid being NULL.
- Bernd Mathiske
On Oct. 3, 2014, 4:46 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2014, 4:46 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
> -----
>
> include/mesos/module.hpp PRE-CREATION
> src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724
> src/examples/example_module_impl.cpp PRE-CREATION
> src/examples/test_module.hpp PRE-CREATION
> src/examples/test_module_impl.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
>
>
Re: Review Request 25848: Introducing mesos modules.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/
-----------------------------------------------------------
(Updated Oct. 3, 2014, 7:46 p.m.)
Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, and Timothy St. Clair.
Changes
-------
Updated design using structs instead of flat symbols and macros.
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 (updated)
-----
include/mesos/module.hpp PRE-CREATION
src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724
src/examples/example_module_impl.cpp PRE-CREATION
src/examples/test_module.hpp PRE-CREATION
src/examples/test_module_impl.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
Re: Review Request 25848: Introducing mesos modules.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
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.
Changes
-------
Addressed some of BenH's comments.
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 (updated)
-----
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
Re: Review Request 25848: Introducing mesos modules.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Oct. 1, 2014, 1: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?
>
> Kapil Arya wrote:
> Bernd/Nik: We have been talking about finding a better name anyways. Any ideas?
I suggested 'kind' before
- Niklas
-----------------------------------------------------------
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
>
>
Re: Review Request 25848: Introducing mesos modules.
Posted by Kapil Arya <ka...@mesosphere.io>.
> 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
>
>
Re: Review Request 25848: Introducing mesos modules.
Posted by Bernd Mathiske <be...@mesosphere.io>.
> 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
>
>
Re: Review Request 25848: Introducing mesos modules.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
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
>
>