You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Kapil Arya <ka...@mesosphere.io> on 2014/12/03 19:13:26 UTC
Review Request 28655: [WIP] Added Master hook for adding task labels
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28655/
-----------------------------------------------------------
Review request for mesos and Niklas Nielsen.
Repository: mesos-git
Description
-------
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.
TODOs before we commit:
0. Split this RR to move Master specific code to a separate RR.
1. Rename "hook" to "decorator" (?)
2. Add a test hook module.
Diffs
-----
src/Makefile.am 1d4ba1c8335eb8106cbccf903eaf3d9fdebdcda2
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 de42f8eb7c3c4ed64fb7fea9f4977e276f4a9043
src/module/hook.hpp PRE-CREATION
src/module/manager.cpp b15b0fc3f056fe29bd4d1acca508d75805ef2e0b
Diff: https://reviews.apache.org/r/28655/diff/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 28655: [WIP] Added Master hook for adding task
labels
Posted by Alexander Rukletsov <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28655/#review64793
-----------------------------------------------------------
src/hook/hook.hpp
<https://reviews.apache.org/r/28655/#comment107557>
Unnecessary include?
- Alexander Rukletsov
On Dec. 9, 2014, 10:20 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28655/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2014, 10:20 p.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> 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.
>
> TODOs before we commit:
> 0. Split this RR to move Master specific code to a separate RR.
> 1. Add a test hook module.
>
>
> Diffs
> -----
>
> src/Makefile.am 86161fe7a8bdd86958d24adb74d434cd92d7dfb8
> 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 1cf2074b78e260bcccf96f4383bc4747b1e75063
> src/module/hook.hpp PRE-CREATION
> src/module/manager.cpp b15b0fc3f056fe29bd4d1acca508d75805ef2e0b
>
> Diff: https://reviews.apache.org/r/28655/diff/
>
>
> Testing
> -------
>
>
> 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 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>.
-----------------------------------------------------------
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
>
>
Re: Review Request 28655: Introduced Mesos Hooks abstraction.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28655/
-----------------------------------------------------------
(Updated Dec. 30, 2014, 6:57 p.m.)
Review request for mesos and Niklas Nielsen.
Changes
-------
Updated title/summary.
Summary (updated)
-----------------
Introduced Mesos Hooks abstraction.
Repository: mesos-git
Description (updated)
-------
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 (updated)
-----
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 (updated)
-------
Added two tests (see https://reviews.apache.org/r/29496/) and ran make check.
Thanks,
Kapil Arya
Re: Review Request 28655: [WIP] Added Master hook for adding task
labels
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28655/
-----------------------------------------------------------
(Updated Dec. 17, 2014, 7:32 p.m.)
Review request for mesos and Niklas Nielsen.
Changes
-------
Fixed more whitespace issues in src/Makefile.am; Addressed Till's comments.
Repository: mesos-git
Description
-------
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.
TODOs before we commit:
0. Split this RR to move Master specific code to a separate RR.
1. Add a test hook module.
Diffs (updated)
-----
src/Makefile.am 86161fe7a8bdd86958d24adb74d434cd92d7dfb8
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 1cf2074b78e260bcccf96f4383bc4747b1e75063
src/module/hook.hpp PRE-CREATION
src/module/manager.cpp b15b0fc3f056fe29bd4d1acca508d75805ef2e0b
Diff: https://reviews.apache.org/r/28655/diff/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 28655: [WIP] Added Master hook for adding task
labels
Posted by Kapil Arya <ka...@mesosphere.io>.
> On Dec. 17, 2014, 7:06 p.m., Till Toenshoff wrote:
> > src/Makefile.am, line 269
> > <https://reviews.apache.org/r/28655/diff/3/?file=789854#file789854line269>
> >
> > I would try to adhere more towards the backslash spacing as used by e.g. "logging/logging.hpp" and fix the others in this section (at least). AFAIK for Makefile.am we use 8 char hard-tabs.
Not sure what you mean by this. I was using tabs only. In any case, fixed some more space issues in the newest diff.
> On Dec. 17, 2014, 7:06 p.m., Till Toenshoff wrote:
> > src/hook/manager.cpp, lines 64-67
> > <https://reviews.apache.org/r/28655/diff/3/?file=789857#file789857line64>
> >
> > Is this going to be a growing list of checks for specific hook modules and add them to the respective hook interfaces?
Yes. It should be easy to update if we can come up with a better scheme.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28655/#review65388
-----------------------------------------------------------
On Dec. 17, 2014, 7:32 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28655/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2014, 7:32 p.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> 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.
>
> TODOs before we commit:
> 0. Split this RR to move Master specific code to a separate RR.
> 1. Add a test hook module.
>
>
> Diffs
> -----
>
> src/Makefile.am 86161fe7a8bdd86958d24adb74d434cd92d7dfb8
> 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 1cf2074b78e260bcccf96f4383bc4747b1e75063
> src/module/hook.hpp PRE-CREATION
> src/module/manager.cpp b15b0fc3f056fe29bd4d1acca508d75805ef2e0b
>
> Diff: https://reviews.apache.org/r/28655/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 28655: [WIP] Added Master hook for adding task
labels
Posted by Till Toenshoff <to...@me.com>.
> On Dec. 18, 2014, 12:06 a.m., Till Toenshoff wrote:
> > src/Makefile.am, line 269
> > <https://reviews.apache.org/r/28655/diff/3/?file=789854#file789854line269>
> >
> > I would try to adhere more towards the backslash spacing as used by e.g. "logging/logging.hpp" and fix the others in this section (at least). AFAIK for Makefile.am we use 8 char hard-tabs.
>
> Kapil Arya wrote:
> Not sure what you mean by this. I was using tabs only. In any case, fixed some more space issues in the newest diff.
Great - I wasnt entirely sure which part exactly was offending our rules it just appeared to be inconsitant even when looking at it with an incorrect tab-setup. Its hard to see properly when using RR.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28655/#review65388
-----------------------------------------------------------
On Dec. 18, 2014, 12:32 a.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28655/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2014, 12:32 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> 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.
>
> TODOs before we commit:
> 0. Split this RR to move Master specific code to a separate RR.
> 1. Add a test hook module.
>
>
> Diffs
> -----
>
> src/Makefile.am 86161fe7a8bdd86958d24adb74d434cd92d7dfb8
> 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 1cf2074b78e260bcccf96f4383bc4747b1e75063
> src/module/hook.hpp PRE-CREATION
> src/module/manager.cpp b15b0fc3f056fe29bd4d1acca508d75805ef2e0b
>
> Diff: https://reviews.apache.org/r/28655/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 28655: [WIP] Added Master hook for adding task
labels
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28655/#review65388
-----------------------------------------------------------
Ship it!
This looks great Kapil!
src/Makefile.am
<https://reviews.apache.org/r/28655/#comment108490>
I would try to adhere more towards the backslash spacing as used by e.g. "logging/logging.hpp" and fix the others in this section (at least). AFAIK for Makefile.am we use 8 char hard-tabs.
src/hook/manager.cpp
<https://reviews.apache.org/r/28655/#comment108493>
Is this going to be a growing list of checks for specific hook modules and add them to the respective hook interfaces?
src/master/flags.hpp
<https://reviews.apache.org/r/28655/#comment108489>
Isnt lowercase "comma" just fine?
- Till Toenshoff
On Dec. 11, 2014, 11:23 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28655/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2014, 11:23 p.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> 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.
>
> TODOs before we commit:
> 0. Split this RR to move Master specific code to a separate RR.
> 1. Add a test hook module.
>
>
> Diffs
> -----
>
> src/Makefile.am 86161fe7a8bdd86958d24adb74d434cd92d7dfb8
> 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 1cf2074b78e260bcccf96f4383bc4747b1e75063
> src/module/hook.hpp PRE-CREATION
> src/module/manager.cpp b15b0fc3f056fe29bd4d1acca508d75805ef2e0b
>
> Diff: https://reviews.apache.org/r/28655/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 28655: [WIP] Added Master hook for adding task
labels
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28655/
-----------------------------------------------------------
(Updated Dec. 11, 2014, 6:23 p.m.)
Review request for mesos and Niklas Nielsen.
Changes
-------
Removed a stale #include.
Repository: mesos-git
Description
-------
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.
TODOs before we commit:
0. Split this RR to move Master specific code to a separate RR.
1. Add a test hook module.
Diffs (updated)
-----
src/Makefile.am 86161fe7a8bdd86958d24adb74d434cd92d7dfb8
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 1cf2074b78e260bcccf96f4383bc4747b1e75063
src/module/hook.hpp PRE-CREATION
src/module/manager.cpp b15b0fc3f056fe29bd4d1acca508d75805ef2e0b
Diff: https://reviews.apache.org/r/28655/diff/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 28655: [WIP] Added Master hook for adding task
labels
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28655/
-----------------------------------------------------------
(Updated Dec. 9, 2014, 5:20 p.m.)
Review request for mesos and Niklas Nielsen.
Changes
-------
Addressed Nik's comments.
Repository: mesos-git
Description (updated)
-------
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.
TODOs before we commit:
0. Split this RR to move Master specific code to a separate RR.
1. Add a test hook module.
Diffs (updated)
-----
src/Makefile.am 86161fe7a8bdd86958d24adb74d434cd92d7dfb8
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 1cf2074b78e260bcccf96f4383bc4747b1e75063
src/module/hook.hpp PRE-CREATION
src/module/manager.cpp b15b0fc3f056fe29bd4d1acca508d75805ef2e0b
Diff: https://reviews.apache.org/r/28655/diff/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 28655: [WIP] Added Master hook for adding task
labels
Posted by Kapil Arya <ka...@mesosphere.io>.
> On Dec. 5, 2014, 5:49 p.m., Niklas Nielsen wrote:
> > src/hook/hook.hpp, line 36
> > <https://reviews.apache.org/r/28655/diff/1/?file=781773#file781773line36>
> >
> > Think a namespace reads better for task label vs environment decorator; that way we don't have to name it masterXXXXXXLabelDecorator vs masterXXXXXXEnvironmentDecorator
> >
> > What do you think?
I think namespaces add more complexity, at least right now. We can revisit the decision at a later time.
> On Dec. 5, 2014, 5:49 p.m., Niklas Nielsen wrote:
> > src/hook/manager.hpp, line 35
> > <https://reviews.apache.org/r/28655/diff/1/?file=781774#file781774line35>
> >
> > I don't understand this enum - can you expand on what you want to use it for?
This will be more apparent once we have more than one hook/decorator.
> On Dec. 5, 2014, 5:49 p.m., Niklas Nielsen wrote:
> > src/hook/manager.hpp, line 38
> > <https://reviews.apache.org/r/28655/diff/1/?file=781774#file781774line38>
> >
> > NUM_HOOKS?
Removed.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28655/#review63916
-----------------------------------------------------------
On Dec. 9, 2014, 5:20 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28655/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2014, 5:20 p.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> 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.
>
> TODOs before we commit:
> 0. Split this RR to move Master specific code to a separate RR.
> 1. Add a test hook module.
>
>
> Diffs
> -----
>
> src/Makefile.am 86161fe7a8bdd86958d24adb74d434cd92d7dfb8
> 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 1cf2074b78e260bcccf96f4383bc4747b1e75063
> src/module/hook.hpp PRE-CREATION
> src/module/manager.cpp b15b0fc3f056fe29bd4d1acca508d75805ef2e0b
>
> Diff: https://reviews.apache.org/r/28655/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 28655: [WIP] Added Master hook for adding task
labels
Posted by Kapil Arya <ka...@mesosphere.io>.
> On Dec. 5, 2014, 5:49 p.m., Niklas Nielsen wrote:
> > src/master/flags.hpp, line 361
> > <https://reviews.apache.org/r/28655/diff/1/?file=781776#file781776line361>
> >
> > Include example
We don't have examples for other flags that accept comma-seperated values so I guess we should skip here too :-).
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28655/#review63916
-----------------------------------------------------------
On Dec. 17, 2014, 7:32 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28655/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2014, 7:32 p.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> 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.
>
> TODOs before we commit:
> 0. Split this RR to move Master specific code to a separate RR.
> 1. Add a test hook module.
>
>
> Diffs
> -----
>
> src/Makefile.am 86161fe7a8bdd86958d24adb74d434cd92d7dfb8
> 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 1cf2074b78e260bcccf96f4383bc4747b1e75063
> src/module/hook.hpp PRE-CREATION
> src/module/manager.cpp b15b0fc3f056fe29bd4d1acca508d75805ef2e0b
>
> Diff: https://reviews.apache.org/r/28655/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 28655: [WIP] Added Master hook for adding task
labels
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28655/#review63916
-----------------------------------------------------------
Much better :-)
Yes, let's rename and prepare for both task label and environment decorators.
The test label and environment decorators should be straight forward, adding and removing labels in the module code and verify the effect in endpoints/at the executor end.
src/Makefile.am
<https://reviews.apache.org/r/28655/#comment106241>
Fly by changes?
src/hook/hook.hpp
<https://reviews.apache.org/r/28655/#comment106527>
Decorator or TaskDecorator or maybe put it in a namespace?
src/hook/hook.hpp
<https://reviews.apache.org/r/28655/#comment106531>
Think a namespace reads better for task label vs environment decorator; that way we don't have to name it masterXXXXXXLabelDecorator vs masterXXXXXXEnvironmentDecorator
What do you think?
src/hook/manager.hpp
<https://reviews.apache.org/r/28655/#comment106532>
I don't understand this enum - can you expand on what you want to use it for?
src/hook/manager.hpp
<https://reviews.apache.org/r/28655/#comment106242>
NUM_HOOKS?
src/hook/manager.cpp
<https://reviews.apache.org/r/28655/#comment106534>
Use the Labels message and do ->MergeFrom. That should just append the labels. We have to think about what we do to *remove* labels (and we need a test for it).
src/master/flags.hpp
<https://reviews.apache.org/r/28655/#comment106243>
Include example
src/master/master.cpp
<https://reviews.apache.org/r/28655/#comment106539>
We should be able to call it just before send() right? We can't influence the call sequence anyway and should only be invoked with validation succeeded, right?
src/master/master.cpp
<https://reviews.apache.org/r/28655/#comment106536>
MergeFrom should work on labels now
- Niklas Nielsen
On Dec. 3, 2014, 10:13 a.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28655/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2014, 10:13 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> 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.
>
> TODOs before we commit:
> 0. Split this RR to move Master specific code to a separate RR.
> 1. Rename "hook" to "decorator" (?)
> 2. Add a test hook module.
>
>
> Diffs
> -----
>
> src/Makefile.am 1d4ba1c8335eb8106cbccf903eaf3d9fdebdcda2
> 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 de42f8eb7c3c4ed64fb7fea9f4977e276f4a9043
> src/module/hook.hpp PRE-CREATION
> src/module/manager.cpp b15b0fc3f056fe29bd4d1acca508d75805ef2e0b
>
> Diff: https://reviews.apache.org/r/28655/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>