Re: Review Request 25848: Introducing mesos modules.

2014-10-08 Thread 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,

Re: Review Request 25848: Introducing mesos modules.

2014-10-08 Thread Michael Park
On Oct. 8, 2014, 1:17 a.m., Michael Park wrote: src/module/manager.hpp, lines 95-105 https://reviews.apache.org/r/25848/diff/17/?file=714903#file714903line95 I would suggest changing these to be static functions that return static singletons as per

Re: Review Request 25848: Introducing mesos modules.

2014-10-08 Thread 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,

Re: Review Request 25848: Introducing mesos modules.

2014-10-08 Thread 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.,

Re: Review Request 25848: Introducing mesos modules.

2014-10-08 Thread Niklas Nielsen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25848/#review55874 --- src/module/manager.cpp

Re: Review Request 25848: Introducing mesos modules.

2014-10-08 Thread 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,

Re: Review Request 25848: Introducing mesos modules.

2014-10-08 Thread 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

Re: Review Request 25848: Introducing mesos modules.

2014-10-07 Thread 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,

Re: Review Request 25848: Introducing mesos modules.

2014-10-07 Thread Kapil Arya
On Oct. 6, 2014, 11:56 p.m., Benjamin Hindman wrote: src/examples/example_module_impl.cpp, line 32 https://reviews.apache.org/r/25848/diff/14/?file=713786#file713786line32 To Bernd's point above, if you declare/define this at the bottom then you won't need the forward

Re: Review Request 25848: Introducing mesos modules.

2014-10-07 Thread Kapil Arya
On Oct. 6, 2014, 7:25 a.m., Bernd Mathiske wrote: src/examples/test_module.hpp, line 26 https://reviews.apache.org/r/25848/diff/14/?file=713787#file713787line26 Now there is a confusion whether TestModule is the module or exampleModule is the module. I suggest we only call one of

Re: Review Request 25848: Introducing mesos modules.

2014-10-07 Thread 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,

Re: Review Request 25848: Introducing mesos modules.

2014-10-07 Thread 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,

Re: Review Request 25848: Introducing mesos modules.

2014-10-07 Thread Michael Park
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25848/#review55735 --- src/module/manager.hpp

Re: Review Request 25848: Introducing mesos modules.

2014-10-07 Thread Benjamin Hindman
On Oct. 8, 2014, 1:17 a.m., Michael Park wrote: src/module/manager.hpp, lines 95-105 https://reviews.apache.org/r/25848/diff/17/?file=714903#file714903line95 I would suggest changing these to be static functions that return static singletons as per

Re: Review Request 25848: Introducing mesos modules.

2014-10-06 Thread Bernd Mathiske
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25848/#review55487 --- include/mesos/module.hpp

Re: Review Request 25848: Introducing mesos modules.

2014-10-06 Thread 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

Re: Review Request 25848: Introducing mesos modules.

2014-10-03 Thread Niklas Nielsen
On Sept. 29, 2014, 2:57 p.m., Niklas Nielsen wrote: include/mesos/module.hpp.in, lines 46-47 https://reviews.apache.org/r/25848/diff/11/?file=708522#file708522line46 Can we use existing boost helpers for this? Something like

Re: Review Request 25848: Introducing mesos modules.

2014-10-03 Thread 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,

Re: Review Request 25848: Introducing mesos modules.

2014-10-02 Thread Bernd Mathiske
On Oct. 1, 2014, 1:35 p.m., Benjamin Hindman wrote: include/mesos/module.hpp.in, line 78 https://reviews.apache.org/r/25848/diff/12/?file=709880#file709880line78 I still am not understanding why we have to introduce more complexity with macros. It seems like we could get away

Re: Review Request 25848: Introducing mesos modules.

2014-10-01 Thread Timothy St. Clair
On Sept. 29, 2014, 9:57 p.m., Niklas Nielsen wrote: include/mesos/module.hpp.in, lines 46-47 https://reviews.apache.org/r/25848/diff/11/?file=708522#file708522line46 Can we use existing boost helpers for this? Something like

Re: Review Request 25848: Introducing mesos modules.

2014-10-01 Thread Benjamin Hindman
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25848/#review55065 --- include/mesos/module.hpp.in

Re: Review Request 25848: Introducing mesos modules.

2014-10-01 Thread 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,

Re: Review Request 25848: Introducing mesos modules.

2014-10-01 Thread Kapil Arya
On Oct. 1, 2014, 4:35 p.m., Benjamin Hindman wrote: include/mesos/module.hpp.in, line 42 https://reviews.apache.org/r/25848/diff/12/?file=709880#file709880line42 Why aren't we just using MESOS_VERSION from mesos/mesos.hpp(.in)? I don't like the idea of introducing another macro

Re: Review Request 25848: Introducing mesos modules.

2014-10-01 Thread Niklas Nielsen
On Oct. 1, 2014, 1:35 p.m., Benjamin Hindman wrote: src/module/manager.cpp, line 59 https://reviews.apache.org/r/25848/diff/12/?file=709886#file709886line59 Maybe I'm just getting a big grumpy, but I'm really not in favor of overloading the term 'role' here. We've overloaded

Re: Review Request 25848: Introducing mesos modules.

2014-09-30 Thread Bernd Mathiske
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25848/#review54955 --- include/mesos/module.hpp.in

Re: Review Request 25848: Introducing mesos modules.

2014-09-30 Thread 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

Re: Review Request 25848: Introducing mesos modules.

2014-09-30 Thread Kapil Arya
On Sept. 29, 2014, 5:57 p.m., Niklas Nielsen wrote: include/mesos/module.hpp.in, lines 46-47 https://reviews.apache.org/r/25848/diff/11/?file=708522#file708522line46 Can we use existing boost helpers for this? Something like

Re: Review Request 25848: Introducing mesos modules.

2014-09-29 Thread Timothy St. Clair
On Sept. 24, 2014, 8:05 p.m., Timothy St. Clair wrote: src/module/manager.cpp, line 173 https://reviews.apache.org/r/25848/diff/5/?file=701887#file701887line173 You could probably stick the json parsing into a separate sub-class. I'm all for breaking out a small JIRA tree from

Re: Review Request 25848: Introducing mesos modules.

2014-09-29 Thread 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

Re: Review Request 25848: Introducing mesos modules.

2014-09-29 Thread 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

Re: Review Request 25848: Introducing mesos modules.

2014-09-29 Thread 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

Re: Review Request 25848: Introducing mesos modules.

2014-09-26 Thread 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

Re: Review Request 25848: Introducing mesos modules.

2014-09-26 Thread Kapil Arya
On Sept. 24, 2014, 4:05 p.m., Timothy St. Clair wrote: include/mesos/module.hpp, line 73 https://reviews.apache.org/r/25848/diff/5/?file=701882#file701882line73 Perhaps we can breakout in another JIRA, but I would to denote both some form of AUTHORING as well as define api's as

Re: Review Request 25848: Introducing mesos modules.

2014-09-26 Thread Kapil Arya
On Sept. 24, 2014, 4:05 p.m., Timothy St. Clair wrote: src/module/manager.hpp, line 141 https://reviews.apache.org/r/25848/diff/5/?file=701886#file701886line141 can't we use std::mutex now? Kapil Arya wrote: The current code relies on common/lock.hpp, that uses pthread

Re: Review Request 25848: Introducing mesos modules.

2014-09-25 Thread 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

Re: Review Request 25848: Introducing mesos modules.

2014-09-25 Thread 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

Re: Review Request 25848: Introducing mesos modules.

2014-09-25 Thread 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. -

Re: Review Request 25848: Introducing mesos modules.

2014-09-24 Thread 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.

Re: Review Request 25848: Introducing mesos modules.

2014-09-24 Thread 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

Re: Review Request 25848: Introducing mesos modules.

2014-09-24 Thread 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

Re: Review Request 25848: Introducing mesos modules.

2014-09-24 Thread Kapil Arya
On Sept. 24, 2014, 4:05 p.m., Timothy St. Clair wrote: src/module/manager.hpp, line 141 https://reviews.apache.org/r/25848/diff/5/?file=701886#file701886line141 can't we use std::mutex now? The current code relies on common/lock.hpp, that uses pthread mutexes. On Sept. 24, 2014,

Re: Review Request 25848: Introducing mesos modules.

2014-09-24 Thread 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

Re: Review Request 25848: Introducing mesos modules.

2014-09-24 Thread Timothy St. Clair
On Sept. 24, 2014, 8:05 p.m., Timothy St. Clair wrote: include/mesos/module.hpp, line 73 https://reviews.apache.org/r/25848/diff/5/?file=701882#file701882line73 Perhaps we can breakout in another JIRA, but I would to denote both some form of AUTHORING as well as define api's as

Re: Review Request 25848: Introducing mesos modules.

2014-09-23 Thread 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

Re: Review Request 25848: Introducing mesos modules.

2014-09-22 Thread 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

Re: Review Request 25848: Introducing mesos modules.

2014-09-22 Thread 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

Re: Review Request 25848: Introducing mesos modules.

2014-09-22 Thread Kapil Arya
On Sept. 19, 2014, 7:39 p.m., Niklas Nielsen wrote: src/module/manager.hpp, line 88 https://reviews.apache.org/r/25848/diff/1/?file=697028#file697028line88 Can we find some ABI documentation that supports this claim and maybe throw in a reference? Added a TODO for now. On

Re: Review Request 25848: Introducing mesos modules.

2014-09-22 Thread 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:

Re: Review Request 25848: Introducing mesos modules.

2014-09-22 Thread Kapil Arya
On Sept. 19, 2014, 7:39 p.m., Niklas Nielsen wrote: src/module/manager.cpp, lines 43-44 https://reviews.apache.org/r/25848/diff/1/?file=697029#file697029line43 Ben, we did the module manager as a singleton. I know it is a uncommon pattern in Mesos in general. Do you have any

Re: Review Request 25848: Introducing mesos modules.

2014-09-22 Thread 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

Re: Review Request 25848: Introducing mesos modules.

2014-09-22 Thread Kapil Arya
On Sept. 19, 2014, 7:39 p.m., Niklas Nielsen wrote: src/examples/test_module.cpp, line 38 https://reviews.apache.org/r/25848/diff/1/?file=697027#file697027line38 You could also throw in a comment here on what function declaration that gets generated i.e. exported symbol name and

Re: Review Request 25848: Introducing mesos modules.

2014-09-22 Thread 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

Review Request 25848: Introducing mesos modules.

2014-09-19 Thread Kapil Arya
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25848/ --- Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, and

Re: Review Request 25848: Introducing mesos modules.

2014-09-19 Thread 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

Re: Review Request 25848: Introducing mesos modules.

2014-09-19 Thread 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