Re: Review Request 25848: Introducing mesos modules.

[Benjamin Hindman]

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

[Kapil Arya]

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

[Niklas Nielsen]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review55874
---



src/module/manager.cpp


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.

[Niklas Nielsen]

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

[Kapil Arya]

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

[Michael Park]

> On Oct. 8, 2014, 1:17 a.m., Michael Park wrote:
> > src/module/manager.hpp, lines 95-105
> > 
> >
> > 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.

[Kapil Arya]

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

[Benjamin Hindman]

> On Oct. 8, 2014, 1:17 a.m., Michael Park wrote:
> > src/module/manager.hpp, lines 95-105
> > 
> >
> > 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.

[Michael Park]

> On Oct. 7, 2014, 11:43 p.m., Michael Park wrote:
> > src/module/manager.hpp, line 75
> > 
> >
> > 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.

[Michael Park]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review55735
---



src/module/manager.hpp


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.

[Kapil Arya]

> On Oct. 7, 2014, 7:43 p.m., Michael Park wrote:
> > src/module/manager.hpp, line 75
> > 
> >
> > 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.

[Michael Park]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review55732
---



src/module/manager.hpp


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.

[Kapil Arya]

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

[Kapil Arya]

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

[Kapil Arya]

> On Oct. 6, 2014, 7:25 a.m., Bernd Mathiske wrote:
> > src/examples/test_module.hpp, line 26
> > 
> >
> > 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
> > 
> >
> > 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. 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.

[Kapil Arya]

> On Oct. 6, 2014, 11:56 p.m., Benjamin Hindman wrote:
> > src/examples/example_module_impl.cpp, line 32
> > 
> >
> > 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 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* 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.

[Kapil Arya]

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

[Benjamin Hindman]

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


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


Please put * next to type name, thanks!



src/module/manager.hpp


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


Use const string& and * on type name please, thanks!



src/module/manager.hpp


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


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

Re: Review Request 25848: Introducing mesos modules.

[Bernd Mathiske]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review55487
---



include/mesos/module.hpp


Aren't we renaming role->kind?



include/mesos/module.hpp


Need to update this text.



include/mesos/module.hpp


I'd put this first to emphasize that it changes less often.



include/mesos/module.hpp


I'd put this first.



include/mesos/module.hpp


I was expecting a "T create();" method declaration here.



include/mesos/module.hpp


Shouldn't this be in a different file?



include/mesos/module.hpp


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


On principle, I prefer avoiding forward declarations. (Extra maintenance 
cost, extra code reading effort.)



src/examples/test_module.hpp


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


Please no forward decls.



src/examples/test_module_impl.cpp


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


We could just place NULL into the ModuleBase struct here, to test yet 
another code branch in the module manager.



src/module/manager.hpp


Suggestion: LibraryModulesMap
Note the extra "s".
The plural iundicates that each library can have multiple modules.



src/module/manager.cpp


Naked string?



src/module/manager.cpp


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


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

[Kapil Arya]

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

[Niklas Nielsen]

> On Sept. 29, 2014, 2:57 p.m., Niklas Nielsen wrote:
> > include/mesos/module.hpp.in, lines 46-47
> > 
> >
> > Can we use existing boost helpers for this? Something like 
> > http://www.boost.org/doc/libs/1_56_0/libs/preprocessor/doc/ref/cat.html
> 
> Kapil Arya wrote:
> Right now this files doesn't #include any other files and since its just 
> one definition, it seemed simpler to just define APPEND and EXPAND_AND_APPEND 
> macros.  If there is a strong preference, we can definitely use BOOST_PP_CAT 
> instead.
> 
> Timothy St. Clair wrote:
> 2¢ - Don't add extra inclusion.

Feel free to drop - the new struct scheme should get rid of it.


- Niklas


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review54879
---


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.

[Bernd Mathiske]

> On Oct. 1, 2014, 1:35 p.m., Benjamin Hindman wrote:
> > include/mesos/module.hpp.in, line 78
> > 
> >
> > 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 
> > 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 example =
> > {
> >   MESOS_VERSION,
> >   MESOS_MODULES_API_VERSION,
> >   "author",
> >   "aut...@email.com",
> >   "This library provides a Foo implementation",
> >   "Foo",
> >   compatible,
> >   create
> > };
> > 
> > Then loading the module is just loading the library name and this 
> > symbol (called 'example') and using that struct to read out the important 
> > details, i.e., ignoring errors:
> > 
> > Module* module = (Module*) dlsym(library, "example");
> > 
> > Foo* foo = dynamic_cast(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

Re: Review Request 25848: Introducing mesos modules.

[Niklas Nielsen]

> On Oct. 1, 2014, 1:35 p.m., Benjamin Hindman wrote:
> > src/module/manager.cpp, line 59
> > 
> >
> > 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.

[Kapil Arya]

> On Oct. 1, 2014, 4:35 p.m., Benjamin Hindman wrote:
> > include/mesos/module.hpp.in, line 42
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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 
> > 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 example =
> > {
> >   MESOS_VERSION,
> >   MESOS_MODULES_API_VERSION,
> >   "author",
> >   "aut...@email.com",
> >   "This library provides a Foo implementation",
> >   "Foo",
> >   compatible,
> >   create
> > };
> > 
> > Then loading the module is just loading the library name and this 
> > symbol (called 'example') and using that struct to read out the important 
> > details, i.e., ignoring errors:
> > 
> > Module* module = (Module*) dlsym(library, "example");
> > 
> > Foo* foo = dynamic_cast(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 a

Re: Review Request 25848: Introducing mesos modules.

[Kapil Arya]

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

[Benjamin Hindman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review55065
---



include/mesos/module.hpp.in


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


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 
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 example =
{
  MESOS_VERSION,
  MESOS_MODULES_API_VERSION,
  "author",
  "aut...@email.com",
  "This library provides a Foo implementation",
  "Foo",
  compatible,
  create
};

Then loading the module is just loading the library name and this symbol 
(called 'example') and using that struct to read out the important details, 
i.e., ignoring errors:

Module* module = (Module*) dlsym(library, "example");

Foo* foo = dynamic_cast(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


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


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


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


This function is not used.



src/module/manager.hpp


Why introduce the singleton instead of just making all the fields static? 
Seems like we could simplify this.



src/module/manager.hpp


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


Maybe I'm just getting a big grumpy, but I'm rea

Re: Review Request 25848: Introducing mesos modules.

[Timothy St. Clair]

> On Sept. 29, 2014, 9:57 p.m., Niklas Nielsen wrote:
> > include/mesos/module.hpp.in, lines 46-47
> > 
> >
> > Can we use existing boost helpers for this? Something like 
> > http://www.boost.org/doc/libs/1_56_0/libs/preprocessor/doc/ref/cat.html
> 
> Kapil Arya wrote:
> Right now this files doesn't #include any other files and since its just 
> one definition, it seemed simpler to just define APPEND and EXPAND_AND_APPEND 
> macros.  If there is a strong preference, we can definitely use BOOST_PP_CAT 
> instead.

2¢ - Don't add extra inclusion.


- Timothy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review54879
---


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

Re: Review Request 25848: Introducing mesos modules.

[Kapil Arya]

> On Sept. 29, 2014, 5:57 p.m., Niklas Nielsen wrote:
> > include/mesos/module.hpp.in, lines 46-47
> > 
> >
> > Can we use existing boost helpers for this? Something like 
> > http://www.boost.org/doc/libs/1_56_0/libs/preprocessor/doc/ref/cat.html

Right now this files doesn't #include any other files and since its just one 
definition, it seemed simpler to just define APPEND and EXPAND_AND_APPEND 
macros.  If there is a strong preference, we can definitely use BOOST_PP_CAT 
instead.


- Kapil


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review54879
---


On Sept. 30, 2014, 7: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, 7: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
> 
>

Re: Review Request 25848: Introducing mesos modules.

[Kapil Arya]

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

[Bernd Mathiske]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review54955
---



include/mesos/module.hpp.in


MESOS_GET_AUTHOR_NAME -> MESOS_MODULE_AUTHOR_NAME
Same for the next two items.



src/module/manager.cpp


Good thinking. MESOS_VERSION should be the default.


- Bernd Mathiske


On Sept. 29, 2014, 2:16 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> ---
> 
> (Updated Sept. 29, 2014, 2:16 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
> 
>

Re: Review Request 25848: Introducing mesos modules.

[Niklas Nielsen]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review54879
---


Style review.

One high-level comment: the additional fields for authoring doesn't seem to 
scale well - how about introducing a struct for that payload. In other words, 
let's try to limit the use of macros to a bare minimum.


include/mesos/module.hpp.in


Can we use existing boost helpers for this? Something like 
http://www.boost.org/doc/libs/1_56_0/libs/preprocessor/doc/ref/cat.html



include/mesos/module.hpp.in


You can wrap all the functions in extern "C" {}



src/Makefile.am


Kill spaces - hard tabs in makefiles



src/examples/test_module.hpp


{ on new line



src/examples/test_module_impl.cpp


newline between includes from mesos, stout, ...



src/examples/test_module_impl.cpp


Wrap according to 
http://mesos.apache.org/documentation/latest/mesos-c++-style-guide/



src/examples/test_module_impl2.cpp


Newline between the includes



src/examples/test_module_impl2.cpp


Can we avoid complex types in global scope?

And/or have const strings at least?



src/examples/test_module_impl2.cpp


You can mark the whole block (all the MESOS_ macros) as extern "C" to 
avoid duplication:

extern "C" {
...
}

Opposed to extern "C" every time



src/module/manager.hpp


Any particular reason for both using hashmap and maps?



src/module/manager.hpp


const string&? Here and below



src/module/manager.hpp


can we have const strings as keys?



src/module/manager.cpp


If we wanted to be pedantic from the start, how about setting all to 
MESOS_VERSION (like we discussed in the dev@ mailing thread) until we get 
comforatble with the api's stability?



src/module/manager.cpp


Mind indent according to 
http://mesos.apache.org/documentation/latest/mesos-c++-style-guide/ ?

Bring down on newline and 4 space indent



src/module/manager.cpp


Same here



src/module/manager.cpp


Two newlines.



src/module/manager.cpp


Still a bit too dense - can we break up and comment on how it applies to 
the format defined in the header?



src/module/manager.cpp


Two newlines.



src/module/manager.cpp


can we have the key in libraryToModules be a const string instead?



src/tests/module_tests.cpp


Newline between the two



src/tests/module_tests.cpp


Two newlines between implementing functions



src/tests/module_tests.cpp


Can we have it return a const string?



src/tests/module_tests.cpp


bring this down on newline and indent 2



src/tests/module_tests.cpp


See above - let's work a bit on constness of the strings here and below.



src/tests/module_tests.cpp


const string version?



src/tests/module_tests.cpp


const strings?


- Niklas Nielsen


On Sept. 29, 2014, 2:16 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> ---
> 
> (Updated Sept. 29, 2014, 2:16 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 Mes

Re: Review Request 25848: Introducing mesos modules.

[Kapil Arya]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/
---

(Updated Sept. 29, 2014, 5:16 p.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, and 
Timothy St. Clair.


Changes
---

Some style fixes.


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.

[Timothy St. Clair]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review54811
---

Ship it!


I still think there will be revisions, but I'm good for phase 1.  Thanks for 
all the work guys! 

We can open new JIRA's where needed.

- Timothy St. Clair


On Sept. 26, 2014, 11:26 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> ---
> 
> (Updated Sept. 26, 2014, 11:26 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
> 
>

Re: Review Request 25848: Introducing mesos modules.

[Timothy St. Clair]

> On Sept. 24, 2014, 8:05 p.m., Timothy St. Clair wrote:
> > src/module/manager.cpp, line 173
> > 
> >
> > You could probably stick the json parsing into a separate sub-class.  
> > I'm all for breaking out a small JIRA tree from the comments.
> 
> Niklas Nielsen wrote:
> Is it so we can discuss the format further or because we should address 
> this in different patches?
> 
> Kapil Arya wrote:
> I have created a new private function that does all the parsing. 
> Hopefully, that is more readable now.

+1.


- Timothy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review54425
---


On Sept. 26, 2014, 11:26 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> ---
> 
> (Updated Sept. 26, 2014, 11:26 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
> 
>

Re: Review Request 25848: Introducing mesos modules.

[Kapil Arya]

> On Sept. 24, 2014, 4:05 p.m., Timothy St. Clair wrote:
> > src/module/manager.hpp, line 141
> > 
> >
> > can't we use std::mutex now?
> 
> Kapil Arya wrote:
> The current code relies on common/lock.hpp, that uses pthread mutexes.

Added a TODO to replace pthread mutex with std::mutex in common/lock.[ch]pp.


- Kapil


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review54425
---


On Sept. 26, 2014, 7:26 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> ---
> 
> (Updated Sept. 26, 2014, 7:26 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
> 
>

Re: Review Request 25848: Introducing mesos modules.

[Kapil Arya]

> On Sept. 24, 2014, 4:05 p.m., Timothy St. Clair wrote:
> > include/mesos/module.hpp, line 73
> > 
> >
> > Perhaps we can breakout in another JIRA, but I would to denote both 
> > some form of AUTHORING as well as define api's as EXPERIMENTAL.
> 
> Niklas Nielsen wrote:
> In another JIRA to discuss the format of the macros? Or what should the 
> outcome be of the JIRA thread?
> 
> +1 on having AUTHORING.
> 
> How did you think about incorporating the experimentalness of the api? in 
> the macro name?
> 
> Timothy St. Clair wrote:
> re-experimental, we could probably just append to the end of the name 
> with token pasting, but lets open up a separate JIRA for that.  
> 
> I figure this will take iterations.

Created a Jira ticket https://issues.apache.org/jira/browse/MESOS-1836.


- Kapil


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review54425
---


On Sept. 26, 2014, 7:26 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> ---
> 
> (Updated Sept. 26, 2014, 7:26 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
> 
>

Re: Review Request 25848: Introducing mesos modules.

[Kapil Arya]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/
---

(Updated Sept. 26, 2014, 7:26 p.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, and 
Timothy St. Clair.


Changes
---

Add author info to module library.


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.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review54646
---


Patch looks great!

Reviews applied: [25848]

All tests passed.

- Mesos ReviewBot


On Sept. 26, 2014, 1:27 a.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> ---
> 
> (Updated Sept. 26, 2014, 1:27 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
> -
> 
>   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.

[Kapil Arya]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/
---

(Updated Sept. 25, 2014, 9:27 p.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, and 
Timothy St. Clair.


Changes
---

Rebased to mesos:master.


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.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review54638
---


Bad patch!

Reviews applied: [25848]

Failed command: git apply --index 25848.patch

Error:
 error: patch failed: src/Makefile.am:1104
error: src/Makefile.am: patch does not apply

- Mesos ReviewBot


On Sept. 26, 2014, 12:45 a.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> ---
> 
> (Updated Sept. 26, 2014, 12:45 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
> -
> 
>   configure.ac c4b43911f5f8f651ddf8f2e12c263849e07e8089 
>   include/mesos/module.hpp.in PRE-CREATION 
>   src/Makefile.am 9b973e5503e30180045e270220987ba647da8038 
>   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.

[Kapil Arya]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/
---

(Updated Sept. 25, 2014, 8:45 p.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, and 
Timothy St. Clair.


Changes
---

Added a few negative tests.


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 c4b43911f5f8f651ddf8f2e12c263849e07e8089 
  include/mesos/module.hpp.in PRE-CREATION 
  src/Makefile.am 9b973e5503e30180045e270220987ba647da8038 
  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.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review54491
---


Patch looks great!

Reviews applied: [25848]

All tests passed.

- Mesos ReviewBot


On Sept. 25, 2014, 12:11 a.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> ---
> 
> (Updated Sept. 25, 2014, 12:11 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
> -
> 
>   configure.ac da61c2984bac378d5298b781476da4f0c20375d1 
>   include/mesos/module.hpp.in PRE-CREATION 
>   src/Makefile.am 9b973e5503e30180045e270220987ba647da8038 
>   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.

[Timothy St. Clair]

> On Sept. 24, 2014, 8:05 p.m., Timothy St. Clair wrote:
> > include/mesos/module.hpp, line 73
> > 
> >
> > Perhaps we can breakout in another JIRA, but I would to denote both 
> > some form of AUTHORING as well as define api's as EXPERIMENTAL.
> 
> Niklas Nielsen wrote:
> In another JIRA to discuss the format of the macros? Or what should the 
> outcome be of the JIRA thread?
> 
> +1 on having AUTHORING.
> 
> How did you think about incorporating the experimentalness of the api? in 
> the macro name?

re-experimental, we could probably just append to the end of the name with 
token pasting, but lets open up a separate JIRA for that.  

I figure this will take iterations.


- Timothy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review54425
---


On Sept. 25, 2014, 12:11 a.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> ---
> 
> (Updated Sept. 25, 2014, 12:11 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
> -
> 
>   configure.ac da61c2984bac378d5298b781476da4f0c20375d1 
>   include/mesos/module.hpp.in PRE-CREATION 
>   src/Makefile.am 9b973e5503e30180045e270220987ba647da8038 
>   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.

[Kapil Arya]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/
---

(Updated Sept. 24, 2014, 8:11 p.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, and 
Timothy St. Clair.


Changes
---

Fixed a style issue.


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 da61c2984bac378d5298b781476da4f0c20375d1 
  include/mesos/module.hpp.in PRE-CREATION 
  src/Makefile.am 9b973e5503e30180045e270220987ba647da8038 
  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.

[Kapil Arya]

> On Sept. 24, 2014, 4:05 p.m., Timothy St. Clair wrote:
> > src/module/manager.hpp, line 141
> > 
> >
> > can't we use std::mutex now?

The current code relies on common/lock.hpp, that uses pthread mutexes.


> On Sept. 24, 2014, 4:05 p.m., Timothy St. Clair wrote:
> > src/module/manager.cpp, line 173
> > 
> >
> > You could probably stick the json parsing into a separate sub-class.  
> > I'm all for breaking out a small JIRA tree from the comments.
> 
> Niklas Nielsen wrote:
> Is it so we can discuss the format further or because we should address 
> this in different patches?

I have created a new private function that does all the parsing. Hopefully, 
that is more readable now.


- Kapil


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review54425
---


On Sept. 24, 2014, 7:54 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> ---
> 
> (Updated Sept. 24, 2014, 7:54 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 da61c2984bac378d5298b781476da4f0c20375d1 
>   include/mesos/module.hpp.in PRE-CREATION 
>   src/Makefile.am 9b973e5503e30180045e270220987ba647da8038 
>   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.

[Kapil Arya]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/
---

(Updated Sept. 24, 2014, 7:54 p.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, and 
Timothy St. Clair.


Changes
---

Addressed some concerns raised by Adam and Tim.


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 da61c2984bac378d5298b781476da4f0c20375d1 
  include/mesos/module.hpp.in PRE-CREATION 
  src/Makefile.am 9b973e5503e30180045e270220987ba647da8038 
  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.

[Niklas Nielsen]

> On Sept. 24, 2014, 1:05 p.m., Timothy St. Clair wrote:
> > I still debate whether we should name it "plugin" vs. "module".  Thoughts?

Could be called either or, but think the term 'module' is pretty clear and is 
what we have gone with so far. I don't see any good reason to change it now 
unless anyone have some good references why it should be called 'plugins' (and 
not modules).

+1 on your comments - threw in a couple of questions.


> On Sept. 24, 2014, 1:05 p.m., Timothy St. Clair wrote:
> > include/mesos/module.hpp, line 73
> > 
> >
> > Perhaps we can breakout in another JIRA, but I would to denote both 
> > some form of AUTHORING as well as define api's as EXPERIMENTAL.

In another JIRA to discuss the format of the macros? Or what should the outcome 
be of the JIRA thread?

+1 on having AUTHORING.

How did you think about incorporating the experimentalness of the api? in the 
macro name?


> On Sept. 24, 2014, 1:05 p.m., Timothy St. Clair wrote:
> > src/module/manager.cpp, line 173
> > 
> >
> > You could probably stick the json parsing into a separate sub-class.  
> > I'm all for breaking out a small JIRA tree from the comments.

Is it so we can discuss the format further or because we should address this in 
different patches?


- Niklas


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review54425
---


On Sept. 22, 2014, 9:05 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> ---
> 
> (Updated Sept. 22, 2014, 9:05 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 9b973e5503e30180045e270220987ba647da8038 
>   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.

[Timothy St. Clair]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review54425
---


I still debate whether we should name it "plugin" vs. "module".  Thoughts?


include/mesos/module.hpp


If this is just for version information perhaps we can @VERSION@ replace, 
vs. including the whole protobuf header chain.



include/mesos/module.hpp


Perhaps we can breakout in another JIRA, but I would to denote both some 
form of AUTHORING as well as define api's as EXPERIMENTAL.



src/examples/test_module.hpp


+1 re: Adams comment.



src/module/manager.hpp


If it's a singleton typically you can make the constructor 
protected/private such that it can never be instantiated externally.



src/module/manager.hpp


can't we use std::mutex now?



src/module/manager.cpp


same comment as above.



src/module/manager.cpp


You could probably stick the json parsing into a separate sub-class.  I'm 
all for breaking out a small JIRA tree from the comments.


- Timothy St. Clair


On Sept. 23, 2014, 4:05 a.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> ---
> 
> (Updated Sept. 23, 2014, 4:05 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 9b973e5503e30180045e270220987ba647da8038 
>   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.

[Adam B]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review54391
---


Minor comments in passing.


src/examples/test_module.hpp


Not sure why these includes are needed here.



src/examples/test_module_impl.cpp


How about the module "interface" instead of "role"?


- Adam B


On Sept. 22, 2014, 9:05 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> ---
> 
> (Updated Sept. 22, 2014, 9:05 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 9b973e5503e30180045e270220987ba647da8038 
>   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.

[Kapil Arya]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/
---

(Updated Sept. 23, 2014, 12:05 a.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, and 
Timothy St. Clair.


Changes
---

Fixed minor typo.


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 9b973e5503e30180045e270220987ba647da8038 
  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.

[Kapil Arya]

> On Sept. 19, 2014, 7:39 p.m., Niklas Nielsen wrote:
> > src/examples/test_module.cpp, line 38
> > 
> >
> > You could also throw in a comment here on what function declaration 
> > that gets generated i.e. exported symbol name and return type.

Done!


> On Sept. 19, 2014, 7:39 p.m., Niklas Nielsen wrote:
> > src/module/manager.cpp, lines 189-215
> > 
> >
> > This block is very dense - mind spreading it out a little bit?

Added a TODO for Bernd.


> On Sept. 19, 2014, 7:39 p.m., Niklas Nielsen wrote:
> > src/tests/module.hpp, line 29
> > 
> >
> > And perhaps highlight the virtual destructor's importance (in order to 
> > clean up the object in the library's context). People are probably going to 
> > use this as a template for writing new modules :-)

Done.


- Kapil


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review54043
---


On Sept. 22, 2014, 8:22 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> ---
> 
> (Updated Sept. 22, 2014, 8:22 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 9b973e5503e30180045e270220987ba647da8038 
>   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.

[Kapil Arya]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/
---

(Updated Sept. 22, 2014, 8:22 p.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, and 
Timothy St. Clair.


Changes
---

Added comments; use shared_ptr; added lock; and addressed some more 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)
-

  include/mesos/module.hpp PRE-CREATION 
  src/Makefile.am 9b973e5503e30180045e270220987ba647da8038 
  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.

[Kapil Arya]

> On Sept. 19, 2014, 7:39 p.m., Niklas Nielsen wrote:
> > src/module/manager.cpp, lines 43-44
> > 
> >
> > Ben, we did the module manager as a singleton. I know it is a uncommon 
> > pattern in Mesos in general. Do you have any thoughts on this? We will need 
> > to take care of thread-safety with whatever state we maintain in the module 
> > manager.

Ben, any thoughts on this one?


- Kapil


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review54043
---


On Sept. 22, 2014, 8:22 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> ---
> 
> (Updated Sept. 22, 2014, 8:22 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 9b973e5503e30180045e270220987ba647da8038 
>   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.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review54118
---


Bad patch!

Reviews applied: [25848]

Failed command: ./support/mesos-style.py

Error:
 Checking 513 files using filter 
--filter=-,+build/class,+build/deprecated,+build/endif_comment,+readability/todo,+readability/namespace,+runtime/vlog,+whitespace/blank_line,+whitespace/comma,+whitespace/end_of_line,+whitespace/ending_newline,+whitespace/forcolon,+whitespace/indent,+whitespace/line_length,+whitespace/tab,+whitespace/todo
src/module/manager.hpp:85:  Redundant blank line at the end of a code block 
should be deleted.  [whitespace/blank_line] [3]
Total errors found: 1

- Mesos ReviewBot


On Sept. 22, 2014, 9:30 a.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> ---
> 
> (Updated Sept. 22, 2014, 9:30 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 9b973e5503e30180045e270220987ba647da8038 
>   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.

[Kapil Arya]

> On Sept. 19, 2014, 7:39 p.m., Niklas Nielsen wrote:
> > src/module/manager.hpp, line 88
> > 
> >
> > Can we find some ABI documentation that supports this claim and maybe 
> > throw in a reference?

Added a TODO for now.


> On Sept. 19, 2014, 7:39 p.m., Niklas Nielsen wrote:
> > src/module/manager.hpp, lines 117-118
> > 
> >
> > Access to those need to be guarded, right? Or do you explicitally call 
> > out that the module manager isn't thread safe?

Added a TODO for now.


> On Sept. 19, 2014, 7:39 p.m., Niklas Nielsen wrote:
> > src/slave/flags.hpp, line 328
> > 
> >
> > Let's tie the modules in after the core patch has landed. Also, we need 
> > to it for both masters and slaves (alongside starting to wire up module 
> > loading for isolators, authenticators, ...)
> > 
> > TL;DR Let's review the src/slave/flags.hpp and src/slave/main.cpp 
> > changes (with a master equivalent) in a dependent patch.
> > 
> > In the new review, please expand the help text and throw in some 
> > examples. You can put in the expected format as you listed in the module 
> > manager header.

Removed this code for now.  Will create a separate patch and address these 
concerns there.


> On Sept. 19, 2014, 7:39 p.m., Niklas Nielsen wrote:
> > src/tests/module_tests.cpp, line 123
> > 
> >
> > Where are we going to capture the negative tests? Wrong library 
> > versions, trying to load non-mesos modules libraries (libc.so for example)?

Added a TODO for now.


- Kapil


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review54043
---


On Sept. 22, 2014, 5:30 a.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25848/
> ---
> 
> (Updated Sept. 22, 2014, 5:30 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 9b973e5503e30180045e270220987ba647da8038 
>   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.

[Kapil Arya]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/
---

(Updated Sept. 22, 2014, 5:30 a.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, and 
Timothy St. Clair.


Changes
---

Do not tie in modules into slave in the first patch.


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 9b973e5503e30180045e270220987ba647da8038 
  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.

[Kapil Arya]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/
---

(Updated Sept. 22, 2014, 5:25 a.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, and 
Timothy St. Clair.


Changes
---

Fixed compilation errors; addressed some 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)
-

  include/mesos/module.hpp PRE-CREATION 
  src/Makefile.am 9b973e5503e30180045e270220987ba647da8038 
  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/slave/flags.hpp 21e00212bc402674eaea73b44b3f91df477a7213 
  src/slave/main.cpp 2c4d365a04acbcb382e978d811a318130484b3d5 
  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.

[Niklas Nielsen]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review54043
---


First pass


src/examples/test_module.cpp


The build bot failed this include - mind take a look?



src/examples/test_module.cpp


This file will probably serve as a template for new templates, so let's add 
some comments on what the MESOS_MODULE_LIBRARY() and MESOS_MODULE() macros do.



src/examples/test_module.cpp


Mind throwing in a comment on what/where the example module is used?



src/examples/test_module.cpp


You could also throw in a comment here on what function declaration that 
gets generated i.e. exported symbol name and return type.



src/module/manager.hpp


Mind mentioning where you get this configuration from i.e. "--modules"



src/module/manager.hpp


{ on it's own line



src/module/manager.hpp


End comment with period



src/module/manager.hpp


Isn't "module" implied by the module manager? How about ->load() and 
->contains() ?



src/module/manager.hpp


Can we find some ABI documentation that supports this claim and maybe throw 
in a reference?



src/module/manager.hpp


{ on it's own line



src/module/manager.hpp


Newline between loadModuleLibrary and verify module role.

Also, isn't 'module' implied? loadLibrary / verifyRole would be a bit more 
concise.



src/module/manager.hpp


You can use 'CHECK_NOTNULL(lib)->' to guard the libary pointer.



src/module/manager.hpp


Access to those need to be guarded, right? Or do you explicitally call out 
that the module manager isn't thread safe?



src/module/manager.hpp


How about using shared_ptr's instead?

If not, how is cleaning up all the allocated libraries?



src/module/manager.cpp


newline after 'version.hpp'



src/module/manager.cpp


Ben, we did the module manager as a singleton. I know it is a uncommon 
pattern in Mesos in general. Do you have any thoughts on this? We will need to 
take care of thread-safety with whatever state we maintain in the module 
manager.



src/module/manager.cpp


How about using shared_ptr instead of a raw pointer?



src/module/manager.cpp


Mind throwing this on a new line and indent 4 spaces? Here and below



src/module/manager.cpp


This block is very dense - mind spreading it out a little bit?



src/module/manager.cpp


Odd indentation here.



src/slave/flags.hpp


Let's tie the modules in after the core patch has landed. Also, we need to 
it for both masters and slaves (alongside starting to wire up module loading 
for isolators, authenticators, ...)

TL;DR Let's review the src/slave/flags.hpp and src/slave/main.cpp changes 
(with a master equivalent) in a dependent patch.

In the new review, please expand the help text and throw in some examples. 
You can put in the expected format as you listed in the module manager header.



src/slave/main.cpp


See comment above.



src/tests/module.hpp


Mind throwing in a comment on where this is implemented / how it is used?



src/tests/module.hpp


And perhaps highlight the virtual destructor's importance (in order to 
clean up the object in the library's context). People are probably going to use 
this as a template for writing new modules :-)



src/tests/module_tests.cpp


This seems to be repeated a couple of times - can we centralize in a local 
helper?



src/tests/module_tests.cpp


When are you cleaning up this file? Use mktemp instead and kill the file 
when you are done :-)



src/tests/mo

Re: Review Request 25848: Introducing mesos modules.

[Kapil Arya]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/
---

(Updated Sept. 19, 2014, 4:40 p.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, and 
Timothy St. Clair.


Changes
---

Added MESOS-1384 in Bugs field.


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 9b973e5503e30180045e270220987ba647da8038 
  src/examples/test_module.cpp PRE-CREATION 
  src/module/manager.hpp PRE-CREATION 
  src/module/manager.cpp PRE-CREATION 
  src/slave/flags.hpp 21e00212bc402674eaea73b44b3f91df477a7213 
  src/slave/main.cpp 2c4d365a04acbcb382e978d811a318130484b3d5 
  src/tests/module.hpp 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