Re: Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically

[Jay Guo]

> On Oct. 21, 2015, 7:02 a.m., Klaus Ma wrote:
> > src/module/manager.hpp, line 94
> > 
> >
> > Should we merge with `moduleParameters[moduleName]` or replace it? IMO, 
> > prefer to merge them; I'd like to leave it to you and your shepherd.
> 
> Alexander Rojas wrote:
> I don't understand what you mean with this comment? is the same as the 
> following one?
> 
> Klaus Ma wrote:
> for example, if `{a:1, b:2}` in `moduleParameters[moduleName]` and `{b:3, 
> c:4}` in `params`, what's expected result? `{a:1, b:3, c:4}` or `{b:3, c:4}`.
> 
> Alexander Rojas wrote:
> Merge is not a good idea almost ever. In this case, if you have a 
> parameter which can appear once, and the user gave `foo=5` and the API gave 
> `foo=10` which one to choose?

Fairly an old thread but I do prefer `merge` over `replace`, in case some 
parameters are only available during runtime, e.g. PIDs. Wordaround does exist 
but negative consequences are stated in original story description. Thoughts?


- Jay


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


On Nov. 16, 2015, 10:08 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38627/
> ---
> 
> (Updated Nov. 16, 2015, 10:08 a.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3072
> https://issues.apache.org/jira/browse/MESOS-3072
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allows developers to provide their own parameters when loading modules 
> instead of using the ones provided by the user when loading Mesos. This helps 
> to deal with default modules (those used when the user doesn't provide any), 
> and for testing of the modules.
> 
> 
> Diffs
> -
> 
>   src/examples/example_module_impl.cpp 
> db015cea130701a4e0a6fcb890c79fbb0c02c1ce 
>   src/examples/test_module.hpp 0514963b6100a6e9a834f2b9670cebc310918fb8 
>   src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
>   src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 
>   src/tests/module_tests.cpp 60497aac3200ab9a679a81a593b5bf0d02fd4b50 
> 
> Diff: https://reviews.apache.org/r/38627/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically

[Alexander Rojas]

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

(Updated Nov. 16, 2015, 11:08 a.m.)


Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till 
Toenshoff.


Changes
---

Removes unnecesary dependency.


Bugs: MESOS-3072
https://issues.apache.org/jira/browse/MESOS-3072


Repository: mesos


Description
---

Allows developers to provide their own parameters when loading modules instead 
of using the ones provided by the user when loading Mesos. This helps to deal 
with default modules (those used when the user doesn't provide any), and for 
testing of the modules.


Diffs
-

  src/examples/example_module_impl.cpp db015cea130701a4e0a6fcb890c79fbb0c02c1ce 
  src/examples/test_module.hpp 0514963b6100a6e9a834f2b9670cebc310918fb8 
  src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
  src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 
  src/tests/module_tests.cpp 60497aac3200ab9a679a81a593b5bf0d02fd4b50 

Diff: https://reviews.apache.org/r/38627/diff/


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically

[Bernd Mathiske]

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

Ship it!


Ship It!

- Bernd Mathiske


On Nov. 9, 2015, 8:01 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38627/
> ---
> 
> (Updated Nov. 9, 2015, 8:01 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3072
> https://issues.apache.org/jira/browse/MESOS-3072
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allows developers to provide their own parameters when loading modules 
> instead of using the ones provided by the user when loading Mesos. This helps 
> to deal with default modules (those used when the user doesn't provide any), 
> and for testing of the modules.
> 
> 
> Diffs
> -
> 
>   src/examples/example_module_impl.cpp 
> db015cea130701a4e0a6fcb890c79fbb0c02c1ce 
>   src/examples/test_module.hpp 0514963b6100a6e9a834f2b9670cebc310918fb8 
>   src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
>   src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 
>   src/tests/module_tests.cpp 60497aac3200ab9a679a81a593b5bf0d02fd4b50 
> 
> Diff: https://reviews.apache.org/r/38627/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically

[Bernd Mathiske]

> On Nov. 16, 2015, 1:55 a.m., Bernd Mathiske wrote:
> > src/tests/module_tests.cpp, line 293
> > 
> >
> > This tests whether we can retrieve the parameters that were filled it 
> > back out. Unlike the tests above we don't test if the parameters actually 
> > get used in the module. I suggest adding a test that does this.

I see you only added parameters() to this particular test module. In this case, 
we have proof enough the parameters got where they need to be.


- Bernd


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


On Nov. 9, 2015, 8:01 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38627/
> ---
> 
> (Updated Nov. 9, 2015, 8:01 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3072
> https://issues.apache.org/jira/browse/MESOS-3072
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allows developers to provide their own parameters when loading modules 
> instead of using the ones provided by the user when loading Mesos. This helps 
> to deal with default modules (those used when the user doesn't provide any), 
> and for testing of the modules.
> 
> 
> Diffs
> -
> 
>   src/examples/example_module_impl.cpp 
> db015cea130701a4e0a6fcb890c79fbb0c02c1ce 
>   src/examples/test_module.hpp 0514963b6100a6e9a834f2b9670cebc310918fb8 
>   src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
>   src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 
>   src/tests/module_tests.cpp 60497aac3200ab9a679a81a593b5bf0d02fd4b50 
> 
> Diff: https://reviews.apache.org/r/38627/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically

[Till Toenshoff]

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

Ship it!


Ship It!

- Till Toenshoff


On Nov. 10, 2015, 4:01 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38627/
> ---
> 
> (Updated Nov. 10, 2015, 4:01 a.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3072
> https://issues.apache.org/jira/browse/MESOS-3072
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allows developers to provide their own parameters when loading modules 
> instead of using the ones provided by the user when loading Mesos. This helps 
> to deal with default modules (those used when the user doesn't provide any), 
> and for testing of the modules.
> 
> 
> Diffs
> -
> 
>   src/examples/example_module_impl.cpp 
> db015cea130701a4e0a6fcb890c79fbb0c02c1ce 
>   src/examples/test_module.hpp 0514963b6100a6e9a834f2b9670cebc310918fb8 
>   src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
>   src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 
>   src/tests/module_tests.cpp 60497aac3200ab9a679a81a593b5bf0d02fd4b50 
> 
> Diff: https://reviews.apache.org/r/38627/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically

[Alexander Rojas]

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

(Updated Nov. 10, 2015, 5:01 a.m.)


Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till 
Toenshoff.


Changes
---

Rebase.


Bugs: MESOS-3072
https://issues.apache.org/jira/browse/MESOS-3072


Repository: mesos


Description
---

Allows developers to provide their own parameters when loading modules instead 
of using the ones provided by the user when loading Mesos. This helps to deal 
with default modules (those used when the user doesn't provide any), and for 
testing of the modules.


Diffs (updated)
-

  src/examples/example_module_impl.cpp db015cea130701a4e0a6fcb890c79fbb0c02c1ce 
  src/examples/test_module.hpp 0514963b6100a6e9a834f2b9670cebc310918fb8 
  src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
  src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 
  src/tests/module_tests.cpp 60497aac3200ab9a679a81a593b5bf0d02fd4b50 

Diff: https://reviews.apache.org/r/38627/diff/


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically

[Guangya Liu]

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

Ship it!


Ship It!

- Guangya Liu


On 十月 21, 2015, 12:52 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38627/
> ---
> 
> (Updated 十月 21, 2015, 12:52 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3072
> https://issues.apache.org/jira/browse/MESOS-3072
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allows developers to provide their own parameters when loading modules 
> instead of using the ones provided by the user when loading Mesos. This helps 
> to deal with default modules (those used when the user doesn't provide any), 
> and for testing of the modules.
> 
> 
> Diffs
> -
> 
>   src/examples/example_module_impl.cpp 
> db015cea130701a4e0a6fcb890c79fbb0c02c1ce 
>   src/examples/test_module.hpp 0514963b6100a6e9a834f2b9670cebc310918fb8 
>   src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
>   src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 
>   src/tests/module_tests.cpp 60497aac3200ab9a679a81a593b5bf0d02fd4b50 
> 
> Diff: https://reviews.apache.org/r/38627/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically

[Klaus Ma]

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

Ship it!


Ship It!

- Klaus Ma


On Oct. 21, 2015, 8:52 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38627/
> ---
> 
> (Updated Oct. 21, 2015, 8:52 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3072
> https://issues.apache.org/jira/browse/MESOS-3072
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allows developers to provide their own parameters when loading modules 
> instead of using the ones provided by the user when loading Mesos. This helps 
> to deal with default modules (those used when the user doesn't provide any), 
> and for testing of the modules.
> 
> 
> Diffs
> -
> 
>   src/examples/example_module_impl.cpp 
> db015cea130701a4e0a6fcb890c79fbb0c02c1ce 
>   src/examples/test_module.hpp 0514963b6100a6e9a834f2b9670cebc310918fb8 
>   src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
>   src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 
>   src/tests/module_tests.cpp 60497aac3200ab9a679a81a593b5bf0d02fd4b50 
> 
> Diff: https://reviews.apache.org/r/38627/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically

[Alexander Rojas]

> On Oct. 21, 2015, 9:02 a.m., Klaus Ma wrote:
> > src/module/manager.hpp, line 94
> > 
> >
> > Should we merge with `moduleParameters[moduleName]` or replace it? IMO, 
> > prefer to merge them; I'd like to leave it to you and your shepherd.
> 
> Alexander Rojas wrote:
> I don't understand what you mean with this comment? is the same as the 
> following one?
> 
> Klaus Ma wrote:
> for example, if `{a:1, b:2}` in `moduleParameters[moduleName]` and `{b:3, 
> c:4}` in `params`, what's expected result? `{a:1, b:3, c:4}` or `{b:3, c:4}`.

Merge is not a good idea almost ever. In this case, if you have a parameter 
which can appear once, and the user gave `foo=5` and the API gave `foo=10` 
which one to choose?


- Alexander


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


On Oct. 21, 2015, 2:52 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38627/
> ---
> 
> (Updated Oct. 21, 2015, 2:52 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3072
> https://issues.apache.org/jira/browse/MESOS-3072
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allows developers to provide their own parameters when loading modules 
> instead of using the ones provided by the user when loading Mesos. This helps 
> to deal with default modules (those used when the user doesn't provide any), 
> and for testing of the modules.
> 
> 
> Diffs
> -
> 
>   src/examples/example_module_impl.cpp 
> db015cea130701a4e0a6fcb890c79fbb0c02c1ce 
>   src/examples/test_module.hpp 0514963b6100a6e9a834f2b9670cebc310918fb8 
>   src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
>   src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 
>   src/tests/module_tests.cpp 60497aac3200ab9a679a81a593b5bf0d02fd4b50 
> 
> Diff: https://reviews.apache.org/r/38627/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically

[Niklas Nielsen]

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


LGTM - Kapil, any thoughts?

- Niklas Nielsen


On Oct. 21, 2015, 5:52 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38627/
> ---
> 
> (Updated Oct. 21, 2015, 5:52 a.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3072
> https://issues.apache.org/jira/browse/MESOS-3072
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allows developers to provide their own parameters when loading modules 
> instead of using the ones provided by the user when loading Mesos. This helps 
> to deal with default modules (those used when the user doesn't provide any), 
> and for testing of the modules.
> 
> 
> Diffs
> -
> 
>   src/examples/example_module_impl.cpp 
> db015cea130701a4e0a6fcb890c79fbb0c02c1ce 
>   src/examples/test_module.hpp 0514963b6100a6e9a834f2b9670cebc310918fb8 
>   src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
>   src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 
>   src/tests/module_tests.cpp 60497aac3200ab9a679a81a593b5bf0d02fd4b50 
> 
> Diff: https://reviews.apache.org/r/38627/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically

[Klaus Ma]

> On Oct. 21, 2015, 3:02 p.m., Klaus Ma wrote:
> > src/module/manager.hpp, line 94
> > 
> >
> > Should we merge with `moduleParameters[moduleName]` or replace it? IMO, 
> > prefer to merge them; I'd like to leave it to you and your shepherd.
> 
> Alexander Rojas wrote:
> I don't understand what you mean with this comment? is the same as the 
> following one?

for example, if `{a:1, b:2}` in `moduleParameters[moduleName]` and `{b:3, c:4}` 
in `params`, what's expected result? `{a:1, b:3, c:4}` or `{b:3, c:4}`.


- Klaus


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


On Oct. 21, 2015, 8:52 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38627/
> ---
> 
> (Updated Oct. 21, 2015, 8:52 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3072
> https://issues.apache.org/jira/browse/MESOS-3072
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allows developers to provide their own parameters when loading modules 
> instead of using the ones provided by the user when loading Mesos. This helps 
> to deal with default modules (those used when the user doesn't provide any), 
> and for testing of the modules.
> 
> 
> Diffs
> -
> 
>   src/examples/example_module_impl.cpp 
> db015cea130701a4e0a6fcb890c79fbb0c02c1ce 
>   src/examples/test_module.hpp 0514963b6100a6e9a834f2b9670cebc310918fb8 
>   src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
>   src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 
>   src/tests/module_tests.cpp 60497aac3200ab9a679a81a593b5bf0d02fd4b50 
> 
> Diff: https://reviews.apache.org/r/38627/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically

[Klaus Ma]

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



src/module/manager.hpp (line 94)


Should we merge with `moduleParameters[moduleName]` or replace it? IMO, 
prefer to merge them; I'd like to leave it to you and your shepherd.



src/module/manager.hpp (line 104)


Prefer to add a `Option` parameter to create instead of a new function. 
Such as

static Try create(const std::string& moduleName, const 
Option& params)


- Klaus Ma


On Oct. 20, 2015, 9:52 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38627/
> ---
> 
> (Updated Oct. 20, 2015, 9:52 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3072
> https://issues.apache.org/jira/browse/MESOS-3072
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allows developers to provide their own parameters when loading modules 
> instead of using the ones provided by the user when loading Mesos. This helps 
> to deal with default modules (those used when the user doesn't provide any), 
> and for testing of the modules.
> 
> 
> Diffs
> -
> 
>   src/examples/example_module_impl.cpp 
> db015cea130701a4e0a6fcb890c79fbb0c02c1ce 
>   src/examples/test_module.hpp 0514963b6100a6e9a834f2b9670cebc310918fb8 
>   src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
>   src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 
>   src/tests/module_tests.cpp 60497aac3200ab9a679a81a593b5bf0d02fd4b50 
> 
> Diff: https://reviews.apache.org/r/38627/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically

[Alexander Rojas]

> On Oct. 21, 2015, 9:02 a.m., Klaus Ma wrote:
> > src/module/manager.hpp, line 94
> > 
> >
> > Should we merge with `moduleParameters[moduleName]` or replace it? IMO, 
> > prefer to merge them; I'd like to leave it to you and your shepherd.

I don't understand what you mean with this comment? is the same as the 
following one?


- Alexander


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


On Oct. 20, 2015, 3:52 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38627/
> ---
> 
> (Updated Oct. 20, 2015, 3:52 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3072
> https://issues.apache.org/jira/browse/MESOS-3072
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allows developers to provide their own parameters when loading modules 
> instead of using the ones provided by the user when loading Mesos. This helps 
> to deal with default modules (those used when the user doesn't provide any), 
> and for testing of the modules.
> 
> 
> Diffs
> -
> 
>   src/examples/example_module_impl.cpp 
> db015cea130701a4e0a6fcb890c79fbb0c02c1ce 
>   src/examples/test_module.hpp 0514963b6100a6e9a834f2b9670cebc310918fb8 
>   src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
>   src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 
>   src/tests/module_tests.cpp 60497aac3200ab9a679a81a593b5bf0d02fd4b50 
> 
> Diff: https://reviews.apache.org/r/38627/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically

[Alexander Rojas]

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

(Updated Oct. 21, 2015, 2:52 p.m.)


Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till 
Toenshoff.


Changes
---

Adresses issues by Klaus's review.


Bugs: MESOS-3072
https://issues.apache.org/jira/browse/MESOS-3072


Repository: mesos


Description
---

Allows developers to provide their own parameters when loading modules instead 
of using the ones provided by the user when loading Mesos. This helps to deal 
with default modules (those used when the user doesn't provide any), and for 
testing of the modules.


Diffs (updated)
-

  src/examples/example_module_impl.cpp db015cea130701a4e0a6fcb890c79fbb0c02c1ce 
  src/examples/test_module.hpp 0514963b6100a6e9a834f2b9670cebc310918fb8 
  src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
  src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 
  src/tests/module_tests.cpp 60497aac3200ab9a679a81a593b5bf0d02fd4b50 

Diff: https://reviews.apache.org/r/38627/diff/


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically

[Alexander Rojas]

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

(Updated Oct. 20, 2015, 12:18 p.m.)


Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till 
Toenshoff.


Changes
---

Updating dependencies.


Bugs: MESOS-3072
https://issues.apache.org/jira/browse/MESOS-3072


Repository: mesos


Description
---

Allows developers to provide their own parameters when loading modules instead 
of using the ones provided by the user when loading Mesos. This helps to deal 
with default modules (those used when the user doesn't provide any), and for 
testing of the modules.


Diffs
-

  src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
  src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 

Diff: https://reviews.apache.org/r/38627/diff/


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically

[Alexander Rojas]

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

(Updated Oct. 20, 2015, 3:52 p.m.)


Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till 
Toenshoff.


Changes
---

Adds unit tests of overrided methods.


Bugs: MESOS-3072
https://issues.apache.org/jira/browse/MESOS-3072


Repository: mesos


Description
---

Allows developers to provide their own parameters when loading modules instead 
of using the ones provided by the user when loading Mesos. This helps to deal 
with default modules (those used when the user doesn't provide any), and for 
testing of the modules.


Diffs (updated)
-

  src/examples/example_module_impl.cpp db015cea130701a4e0a6fcb890c79fbb0c02c1ce 
  src/examples/test_module.hpp 0514963b6100a6e9a834f2b9670cebc310918fb8 
  src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
  src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 
  src/tests/module_tests.cpp 60497aac3200ab9a679a81a593b5bf0d02fd4b50 

Diff: https://reviews.apache.org/r/38627/diff/


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically

[Alexander Rojas]

> On Oct. 12, 2015, 2:54 p.m., Marco Massenzio wrote:
> > src/tests/module.hpp, lines 76-77
> > 
> >
> > same comment here - it would be great to have fully-documented, 
> > properly-formatted javadoc here, also explaining what could go into 
> > `parameters` how will this affect creating the module, etc.
> > 
> > while this may all appear obvious to you, it may not be so to external 
> > developers using this code to launch/test modules.
> > 
> > thanks!

Hey Marco. You're wrong saying that external developers will have to use any of 
this functions, they are use only internally. There are also a lot of examples 
on how to write modules, if you look in the 
[src/examples](https://github.com/apache/mesos/tree/bb8409882c1e1b5c7eea2a4528f5d3d33dc639b1/src/examples)
 directory, all the files with suffix `_module.cpp` show how to write modules. 
More documentation on the matter can be found at 
[modules.md](https://github.com/apache/mesos/blob/bb8409882c1e1b5c7eea2a4528f5d3d33dc639b1/docs/modules.md).

I however see your point on the need of documentation for the `ModuleManager` 
and I created the JIRA MESOS-3767 which addresses that. I don't add it in this 
patch, because this patch creates overloads of existing methods and uses the 
same documentation of the previous ones. In order to keep the changes localize 
to what concerns this patch, we agree with my shepherd to add the relevant 
documentation in a future patch.


- Alexander


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


On Oct. 20, 2015, 12:18 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38627/
> ---
> 
> (Updated Oct. 20, 2015, 12:18 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3072
> https://issues.apache.org/jira/browse/MESOS-3072
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allows developers to provide their own parameters when loading modules 
> instead of using the ones provided by the user when loading Mesos. This helps 
> to deal with default modules (those used when the user doesn't provide any), 
> and for testing of the modules.
> 
> 
> Diffs
> -
> 
>   src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
>   src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 
> 
> Diff: https://reviews.apache.org/r/38627/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically

[Alexander Rojas]

> On Oct. 12, 2015, 2:54 p.m., Marco Massenzio wrote:
> > Would it be possible to add a few unit tests, also to show usage patterns? 
> > especially given the absence of any documentation, it's kinda difficult to 
> > figure out how is this "intended to work" and, without tests, whether it 
> > works at all.
> 
> Niklas Nielsen wrote:
> Alex: Ping ^^

Sure, I just haven't had time to look into this yet.


- Alexander


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


On Oct. 2, 2015, 10:11 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38627/
> ---
> 
> (Updated Oct. 2, 2015, 10:11 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3232
> https://issues.apache.org/jira/browse/MESOS-3232
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allows developers to provide their own parameters when loading modules 
> instead of using the ones provided by the user when loading Mesos. This helps 
> to deal with default modules (those used when the user doesn't provide any), 
> and for testing of the modules.
> 
> 
> Diffs
> -
> 
>   src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
>   src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 
> 
> Diff: https://reviews.apache.org/r/38627/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically

[Niklas Nielsen]

> On Oct. 12, 2015, 5:54 a.m., Marco Massenzio wrote:
> > Would it be possible to add a few unit tests, also to show usage patterns? 
> > especially given the absence of any documentation, it's kinda difficult to 
> > figure out how is this "intended to work" and, without tests, whether it 
> > works at all.

Alex: Ping ^^


- Niklas


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


On Oct. 2, 2015, 1:11 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38627/
> ---
> 
> (Updated Oct. 2, 2015, 1:11 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3232
> https://issues.apache.org/jira/browse/MESOS-3232
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allows developers to provide their own parameters when loading modules 
> instead of using the ones provided by the user when loading Mesos. This helps 
> to deal with default modules (those used when the user doesn't provide any), 
> and for testing of the modules.
> 
> 
> Diffs
> -
> 
>   src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
>   src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 
> 
> Diff: https://reviews.apache.org/r/38627/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically

[Guangya Liu]

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

Ship it!


Ship It!

- Guangya Liu


On 十月 2, 2015, 8:11 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38627/
> ---
> 
> (Updated 十月 2, 2015, 8:11 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3232
> https://issues.apache.org/jira/browse/MESOS-3232
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allows developers to provide their own parameters when loading modules 
> instead of using the ones provided by the user when loading Mesos. This helps 
> to deal with default modules (those used when the user doesn't provide any), 
> and for testing of the modules.
> 
> 
> Diffs
> -
> 
>   src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
>   src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 
> 
> Diff: https://reviews.apache.org/r/38627/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically

[Marco Massenzio]

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


Would it be possible to add a few unit tests, also to show usage patterns? 
especially given the absence of any documentation, it's kinda difficult to 
figure out how is this "intended to work" and, without tests, whether it works 
at all.


src/module/manager.hpp (line 102)


would it be possible to have proper, full javadoc for this method (as well 
as the other one)?
these will be used by external module developers, so having them 
well-documented would be really great.



src/tests/module.hpp (lines 76 - 77)


same comment here - it would be great to have fully-documented, 
properly-formatted javadoc here, also explaining what could go into 
`parameters` how will this affect creating the module, etc.

while this may all appear obvious to you, it may not be so to external 
developers using this code to launch/test modules.

thanks!



src/tests/module.hpp (line 80)


(I understand this may not be your choice, but...)

I find `N` as an identifier is a poor choice because (a) it's entirely 
non-obvious *what* is it and (b) I'd expect it anyway to be an `int` of some 
sort.

Could you please consider renaming it to be something more expressive?

thanks!


- Marco Massenzio


On Oct. 2, 2015, 8:11 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38627/
> ---
> 
> (Updated Oct. 2, 2015, 8:11 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3232
> https://issues.apache.org/jira/browse/MESOS-3232
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allows developers to provide their own parameters when loading modules 
> instead of using the ones provided by the user when loading Mesos. This helps 
> to deal with default modules (those used when the user doesn't provide any), 
> and for testing of the modules.
> 
> 
> Diffs
> -
> 
>   src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
>   src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 
> 
> Diff: https://reviews.apache.org/r/38627/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically

[Alexander Rojas]

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

Review request for mesos, Adam B, Bernd Mathiske, and Till Toenshoff.


Bugs: MESOS-3232
https://issues.apache.org/jira/browse/MESOS-3232


Repository: mesos


Description
---

Allows developers to provide their own parameters when loading modules instead 
of using the ones provided by the user when loading Mesos. This helps to deal 
with default modules (those used when the user doesn't provide any), and for 
testing of the modules.


Diffs
-

  src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
  src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 

Diff: https://reviews.apache.org/r/38627/diff/


Testing
---

make check


Thanks,

Alexander Rojas