You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Kapil Arya <ka...@mesosphere.io> on 2015/01/05 23:42:50 UTC
Re: Review Request 28655: Introduced Mesos Hooks abstraction.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28655/
-----------------------------------------------------------
(Updated Jan. 5, 2015, 5:42 p.m.)
Review request for mesos and Niklas Nielsen.
Changes
-------
Added jira ticket.
Bugs: MESOS-2060
https://issues.apache.org/jira/browse/MESOS-2060
Repository: mesos-git
Description
-------
Hooks are implemented as Mesos modules. The first hook is a task label-decorator that is called during the task launch sequence in Mesos master.
This hook allows hook modules to add additional labels to the incoming TaskInfo object. The labels are then read on the slave/executor side which may then act upon them.
Diffs
-----
src/Makefile.am 0521f5849acc3237a8fa3970c983beab74441586
src/hook/hook.hpp PRE-CREATION
src/hook/manager.hpp PRE-CREATION
src/hook/manager.cpp PRE-CREATION
src/master/flags.hpp 1cea50c02f3ad7de1e1ae91d65d1accdb9af7b03
src/master/main.cpp 193d53f13d8b14638b311cc290b5a5802ea56299
src/master/master.cpp d6651e299ddb73bfdc1b126c474075db6cda8acd
src/module/hook.hpp PRE-CREATION
src/module/manager.cpp 5e779e4c565ee13206cee59e9097499320474187
Diff: https://reviews.apache.org/r/28655/diff/
Testing
-------
Added two tests (see https://reviews.apache.org/r/29496/) and ran make check.
Thanks,
Kapil Arya
Re: Review Request 28655: Introduced Mesos Hooks abstraction.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On Jan. 6, 2015, 8:26 a.m., Niklas Nielsen wrote:
> > Hi Kapil,
> >
> > One high-level observation is that hooks are wired up with function pointers. Can't we use an abstract class with virtual methods instead?
It can be done either way. One of the reasons for using the functions pointers was efficiency. Suppose we have a hundred available hook routines, however, the hook module implements only 4 of them. And if there are 50 such modules, we can easily imagine going through all 50 modules even though they don't implement most hooks.
Having said that, an abstract (that was our initial design) is cleaner and I wouldn't mind doing that either. In that case, the return type for the hooks can be changed to Try and the virtual function definition in the abstract class will simply return Try<Nothing>.
Until we decide on the high level design, I will hold off on replying to other comments to avoid additional noise.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28655/#review66813
-----------------------------------------------------------
On Jan. 5, 2015, 5:42 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28655/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2015, 5:42 p.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2060
> https://issues.apache.org/jira/browse/MESOS-2060
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Hooks are implemented as Mesos modules. The first hook is a task label-decorator that is called during the task launch sequence in Mesos master.
>
> This hook allows hook modules to add additional labels to the incoming TaskInfo object. The labels are then read on the slave/executor side which may then act upon them.
>
>
> Diffs
> -----
>
> src/Makefile.am 0521f5849acc3237a8fa3970c983beab74441586
> src/hook/hook.hpp PRE-CREATION
> src/hook/manager.hpp PRE-CREATION
> src/hook/manager.cpp PRE-CREATION
> src/master/flags.hpp 1cea50c02f3ad7de1e1ae91d65d1accdb9af7b03
> src/master/main.cpp 193d53f13d8b14638b311cc290b5a5802ea56299
> src/master/master.cpp d6651e299ddb73bfdc1b126c474075db6cda8acd
> src/module/hook.hpp PRE-CREATION
> src/module/manager.cpp 5e779e4c565ee13206cee59e9097499320474187
>
> Diff: https://reviews.apache.org/r/28655/diff/
>
>
> Testing
> -------
>
> Added two tests (see https://reviews.apache.org/r/29496/) and ran make check.
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 28655: Introduced Mesos Hooks abstraction.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Jan. 6, 2015, 5:26 a.m., Niklas Nielsen wrote:
> > Hi Kapil,
> >
> > One high-level observation is that hooks are wired up with function pointers. Can't we use an abstract class with virtual methods instead?
>
> Kapil Arya wrote:
> It can be done either way. One of the reasons for using the functions pointers was efficiency. Suppose we have a hundred available hook routines, however, the hook module implements only 4 of them. And if there are 50 such modules, we can easily imagine going through all 50 modules even though they don't implement most hooks.
>
> Having said that, an abstract (that was our initial design) is cleaner and I wouldn't mind doing that either. In that case, the return type for the hooks can be changed to Try and the virtual function definition in the abstract class will simply return Try<Nothing>.
>
> Until we decide on the high level design, I will hold off on replying to other comments to avoid additional noise.
I would optimize for readability, avoiding to check for null function pointers and having a pattern that looks like the other intefaces. The Hook interface can provide default noop-implementations, so the subclassed hook implementations only need to implement what's necessary. Also, being able to deal with error cases is important.
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28655/#review66813
-----------------------------------------------------------
On Jan. 5, 2015, 2:42 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28655/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2015, 2:42 p.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2060
> https://issues.apache.org/jira/browse/MESOS-2060
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Hooks are implemented as Mesos modules. The first hook is a task label-decorator that is called during the task launch sequence in Mesos master.
>
> This hook allows hook modules to add additional labels to the incoming TaskInfo object. The labels are then read on the slave/executor side which may then act upon them.
>
>
> Diffs
> -----
>
> src/Makefile.am 0521f5849acc3237a8fa3970c983beab74441586
> src/hook/hook.hpp PRE-CREATION
> src/hook/manager.hpp PRE-CREATION
> src/hook/manager.cpp PRE-CREATION
> src/master/flags.hpp 1cea50c02f3ad7de1e1ae91d65d1accdb9af7b03
> src/master/main.cpp 193d53f13d8b14638b311cc290b5a5802ea56299
> src/master/master.cpp d6651e299ddb73bfdc1b126c474075db6cda8acd
> src/module/hook.hpp PRE-CREATION
> src/module/manager.cpp 5e779e4c565ee13206cee59e9097499320474187
>
> Diff: https://reviews.apache.org/r/28655/diff/
>
>
> Testing
> -------
>
> Added two tests (see https://reviews.apache.org/r/29496/) and ran make check.
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 28655: Introduced Mesos Hooks abstraction.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28655/#review66813
-----------------------------------------------------------
Hi Kapil,
One high-level observation is that hooks are wired up with function pointers. Can't we use an abstract class with virtual methods instead?
src/hook/hook.hpp
<https://reviews.apache.org/r/28655/#comment110464>
Why is a Hook not a pure abstract class?
Couldn't this be a virtual function and fit the usual pattern better?
src/hook/manager.hpp
<https://reviews.apache.org/r/28655/#comment110465>
newline
src/hook/manager.hpp
<https://reviews.apache.org/r/28655/#comment110471>
Should we make the return type a Try so we can capture hook errors?
src/hook/manager.cpp
<https://reviews.apache.org/r/28655/#comment110470>
Can you make hooks const?
src/hook/manager.cpp
<https://reviews.apache.org/r/28655/#comment110469>
Mind adding some comments on the different steps involved?
src/hook/manager.cpp
<https://reviews.apache.org/r/28655/#comment110449>
Add newline
src/hook/manager.cpp
<https://reviews.apache.org/r/28655/#comment110466>
Don't you need to acquire the lock here? Or else, why have the lock at all?
src/hook/manager.cpp
<https://reviews.apache.org/r/28655/#comment110448>
only 4 space indent.
If the decorator returns a try, you can merge it conditionally
src/master/flags.hpp
<https://reviews.apache.org/r/28655/#comment110467>
Fly by change. Can we postpone it to a different review?
src/master/flags.hpp
<https://reviews.apache.org/r/28655/#comment110468>
Maybe add some examples
src/master/master.cpp
<https://reviews.apache.org/r/28655/#comment110447>
s/newLabels/labels/g
Why an option type? masterLaunchTaskLabelDecorator always returns a Labels in your case. Shouldn't it be a Try instead?
- Niklas Nielsen
On Jan. 5, 2015, 2:42 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28655/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2015, 2:42 p.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2060
> https://issues.apache.org/jira/browse/MESOS-2060
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Hooks are implemented as Mesos modules. The first hook is a task label-decorator that is called during the task launch sequence in Mesos master.
>
> This hook allows hook modules to add additional labels to the incoming TaskInfo object. The labels are then read on the slave/executor side which may then act upon them.
>
>
> Diffs
> -----
>
> src/Makefile.am 0521f5849acc3237a8fa3970c983beab74441586
> src/hook/hook.hpp PRE-CREATION
> src/hook/manager.hpp PRE-CREATION
> src/hook/manager.cpp PRE-CREATION
> src/master/flags.hpp 1cea50c02f3ad7de1e1ae91d65d1accdb9af7b03
> src/master/main.cpp 193d53f13d8b14638b311cc290b5a5802ea56299
> src/master/master.cpp d6651e299ddb73bfdc1b126c474075db6cda8acd
> src/module/hook.hpp PRE-CREATION
> src/module/manager.cpp 5e779e4c565ee13206cee59e9097499320474187
>
> Diff: https://reviews.apache.org/r/28655/diff/
>
>
> Testing
> -------
>
> Added two tests (see https://reviews.apache.org/r/29496/) and ran make check.
>
>
> Thanks,
>
> Kapil Arya
>
>