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:15:04 UTC
Review Request 28656: Added Slave hooks.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28656/
-----------------------------------------------------------
Review request for mesos and Niklas Nielsen.
Repository: mesos-git
Description
-------
Similar to Master hooks.
Diffs
-----
src/hook/hook.hpp PRE-CREATION
src/hook/manager.hpp PRE-CREATION
src/hook/manager.cpp PRE-CREATION
src/slave/flags.hpp fee79e04cca14e136241af06af1fa9105a70b6dd
src/slave/main.cpp 087944aab56685b53772491de39b373ecacd6c57
src/slave/slave.hpp c6d11ef2de77738ab46d7ca02778f7237d230a5c
src/slave/slave.cpp 373c8b44f5bc000da801b0925e1a9c346d9ffabf
Diff: https://reviews.apache.org/r/28656/diff/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 28656: Added Slave hooks.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28656/#review63701
-----------------------------------------------------------
Bad patch!
Reviews applied: [28655]
Failed command: ./support/apply-review.sh -n -r 28655
Error:
2014-12-03 18:36:39 URL:https://reviews.apache.org/r/28655/diff/raw/ [13655/13655] -> "28655.patch" [1]
error: patch failed: src/master/master.cpp:2485
error: src/master/master.cpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On Dec. 3, 2014, 6:15 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28656/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2014, 6:15 p.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Similar to Master hooks.
>
>
> Diffs
> -----
>
> src/hook/hook.hpp PRE-CREATION
> src/hook/manager.hpp PRE-CREATION
> src/hook/manager.cpp PRE-CREATION
> src/slave/flags.hpp fee79e04cca14e136241af06af1fa9105a70b6dd
> src/slave/main.cpp 087944aab56685b53772491de39b373ecacd6c57
> src/slave/slave.hpp c6d11ef2de77738ab46d7ca02778f7237d230a5c
> src/slave/slave.cpp 373c8b44f5bc000da801b0925e1a9c346d9ffabf
>
> Diff: https://reviews.apache.org/r/28656/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 28656: Added environment decorator for Slave.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On Jan. 6, 2015, 8:38 a.m., Niklas Nielsen wrote:
> > src/slave/slave.cpp, line 3969
> > <https://reviews.apache.org/r/28656/diff/4/?file=803961#file803961line3969>
> >
> > Why this refactor? Couldn't you just inline the hook like in the master?
With master label decorator, the merging of labels was done explicitely. However, in this case, the executorInfo is used to create the Executor object and we don't do any explicit merging of environment variables. Further, the refactor is required to presever the constness of the executorInfo variable. If we give up on the constness, we can remove this indirection.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28656/#review66814
-----------------------------------------------------------
On Jan. 13, 2015, 1:24 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28656/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2015, 1:24 p.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2060
> https://issues.apache.org/jira/browse/MESOS-2060
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Similar to label decorator for Master.
>
>
> Diffs
> -----
>
> src/hook/hook.hpp PRE-CREATION
> src/hook/manager.hpp PRE-CREATION
> src/hook/manager.cpp PRE-CREATION
> src/slave/flags.hpp f1b8dfbb7391167b67a9498561742aa9ab9089a6
> src/slave/main.cpp 2ff2b0d186dd63e1437956e98e3682d83f2f5cd6
> src/slave/slave.hpp 70bd8c1fde4ea09fa54c76aa93424a1adb0309f6
> src/slave/slave.cpp b234f5359a91a293b93f97f035b08ef0a4c9b20d
>
> Diff: https://reviews.apache.org/r/28656/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 28656: Added environment decorator for Slave.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28656/#review66814
-----------------------------------------------------------
High-level comments from the previous RR also applies here (abstract class and Try return type).
Another one is the break up of launch task, I'd like to get BenH or BenM's take on it.
src/hook/hook.hpp
<https://reviews.apache.org/r/28656/#comment110472>
From previous RR: Shouldn't we make the return type a Try, so we can ignore the result from failed hooks?
src/slave/flags.hpp
<https://reviews.apache.org/r/28656/#comment110450>
Can you isolate fly-fixes in another review? Here and below
src/slave/slave.cpp
<https://reviews.apache.org/r/28656/#comment110453>
s/newEnvironment/environment/g
You only have one.
src/slave/slave.cpp
<https://reviews.apache.org/r/28656/#comment110474>
Can you find an example where we wrap like this? To distinguish that it is the environment pointer you are evnentually merging, I tend to lean toward either splitting this up (Get the command pointer and then the environment pointer) or just nest with 2 additional spaces for environment and make it on one line.
src/slave/slave.cpp
<https://reviews.apache.org/r/28656/#comment110473>
Why this refactor? Couldn't you just inline the hook like in the master?
- 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/28656/
> -----------------------------------------------------------
>
> (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
> -------
>
> Similar to label decorator for Master.
>
>
> Diffs
> -----
>
> src/hook/hook.hpp PRE-CREATION
> src/hook/manager.hpp PRE-CREATION
> src/hook/manager.cpp PRE-CREATION
> src/slave/flags.hpp 670997dc3a702cd5edf33f2e5824c5e4dfe4ecef
> src/slave/main.cpp 2ff2b0d186dd63e1437956e98e3682d83f2f5cd6
> src/slave/slave.hpp 70bd8c1fde4ea09fa54c76aa93424a1adb0309f6
> src/slave/slave.cpp 50b57819b55bdcdb9f49f20648199badc4d3f37b
>
> Diff: https://reviews.apache.org/r/28656/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 28656: Added environment decorator for Slave.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On Jan. 13, 2015, 5:35 p.m., Niklas Nielsen wrote:
> > src/hook/manager.cpp, lines 104-105
> > <https://reviews.apache.org/r/28656/diff/5/?file=819528#file819528line104>
> >
> > Similar, can we make this an option type?
Updated design doesn't require the bool.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28656/#review67960
-----------------------------------------------------------
On Jan. 13, 2015, 7:36 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28656/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2015, 7:36 p.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2060
> https://issues.apache.org/jira/browse/MESOS-2060
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Similar to label decorator for Master.
>
>
> Diffs
> -----
>
> src/hook/hook.hpp PRE-CREATION
> src/hook/manager.hpp PRE-CREATION
> src/hook/manager.cpp PRE-CREATION
> src/slave/flags.hpp f1b8dfbb7391167b67a9498561742aa9ab9089a6
> src/slave/main.cpp 2ff2b0d186dd63e1437956e98e3682d83f2f5cd6
> src/slave/slave.hpp 70bd8c1fde4ea09fa54c76aa93424a1adb0309f6
> src/slave/slave.cpp b234f5359a91a293b93f97f035b08ef0a4c9b20d
>
> Diff: https://reviews.apache.org/r/28656/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 28656: Added environment decorator for Slave.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28656/#review67960
-----------------------------------------------------------
Also looks good to go modulo the comments below
src/hook/hook.hpp
<https://reviews.apache.org/r/28656/#comment112039>
Let's add an API comment here about the call site, just like the parent RR.
src/hook/manager.cpp
<https://reviews.apache.org/r/28656/#comment112040>
Similar, can we make this an option type?
src/slave/slave.cpp
<https://reviews.apache.org/r/28656/#comment112043>
Let's make this as a helper instead similar to updateGracePeriod()
- Niklas Nielsen
On Jan. 13, 2015, 10:24 a.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28656/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2015, 10:24 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2060
> https://issues.apache.org/jira/browse/MESOS-2060
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Similar to label decorator for Master.
>
>
> Diffs
> -----
>
> src/hook/hook.hpp PRE-CREATION
> src/hook/manager.hpp PRE-CREATION
> src/hook/manager.cpp PRE-CREATION
> src/slave/flags.hpp f1b8dfbb7391167b67a9498561742aa9ab9089a6
> src/slave/main.cpp 2ff2b0d186dd63e1437956e98e3682d83f2f5cd6
> src/slave/slave.hpp 70bd8c1fde4ea09fa54c76aa93424a1adb0309f6
> src/slave/slave.cpp b234f5359a91a293b93f97f035b08ef0a4c9b20d
>
> Diff: https://reviews.apache.org/r/28656/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 28656: Added environment decorator for Slave.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28656/#review68000
-----------------------------------------------------------
src/slave/slave.hpp
<https://reviews.apache.org/r/28656/#comment112134>
Let's do this as a file-static helper :)
src/slave/slave.cpp
<https://reviews.apache.org/r/28656/#comment112135>
How about calling it 'decorateExecutorEnvironment'?
- Niklas Nielsen
On Jan. 13, 2015, 5:31 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28656/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2015, 5:31 p.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2060
> https://issues.apache.org/jira/browse/MESOS-2060
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Similar to label decorator for Master.
>
>
> Diffs
> -----
>
> src/hook/hook.hpp PRE-CREATION
> src/hook/manager.hpp PRE-CREATION
> src/hook/manager.cpp PRE-CREATION
> src/slave/flags.hpp f1b8dfbb7391167b67a9498561742aa9ab9089a6
> src/slave/main.cpp 2ff2b0d186dd63e1437956e98e3682d83f2f5cd6
> src/slave/slave.cpp b234f5359a91a293b93f97f035b08ef0a4c9b20d
>
> Diff: https://reviews.apache.org/r/28656/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 28656: Added environment decorator for Slave.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On Jan. 15, 2015, 5:28 p.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 3890
> > <https://reviews.apache.org/r/28656/diff/9/?file=820347#file820347line3890>
> >
> > Why pull this into a static method instead of doing it inline?
> >
> > Also, shouldn't this be guarded by 'flags.hooks.isSome()' ?
It's require a non-const copy of ExecutorInfo to decorate the environment variables. However, we want to preserve the const-ness of the variable executorInfo for safety purposes. So, this was thought of a safer alternative. If there are strong concerns about inline vs static function, I guess we can change it.
The second point is a valid point. I'll create a new review request to take care of that.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28656/#review68335
-----------------------------------------------------------
On Jan. 13, 2015, 8:31 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28656/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2015, 8:31 p.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2060
> https://issues.apache.org/jira/browse/MESOS-2060
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Similar to label decorator for Master.
>
>
> Diffs
> -----
>
> src/hook/hook.hpp PRE-CREATION
> src/hook/manager.hpp PRE-CREATION
> src/hook/manager.cpp PRE-CREATION
> src/slave/flags.hpp f1b8dfbb7391167b67a9498561742aa9ab9089a6
> src/slave/main.cpp 2ff2b0d186dd63e1437956e98e3682d83f2f5cd6
> src/slave/slave.cpp b234f5359a91a293b93f97f035b08ef0a4c9b20d
>
> Diff: https://reviews.apache.org/r/28656/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 28656: Added environment decorator for Slave.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28656/#review68335
-----------------------------------------------------------
src/slave/slave.cpp
<https://reviews.apache.org/r/28656/#comment112519>
Why pull this into a static method instead of doing it inline?
Also, shouldn't this be guarded by 'flags.hooks.isSome()' ?
- Vinod Kone
On Jan. 14, 2015, 1:31 a.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28656/
> -----------------------------------------------------------
>
> (Updated Jan. 14, 2015, 1:31 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2060
> https://issues.apache.org/jira/browse/MESOS-2060
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Similar to label decorator for Master.
>
>
> Diffs
> -----
>
> src/hook/hook.hpp PRE-CREATION
> src/hook/manager.hpp PRE-CREATION
> src/hook/manager.cpp PRE-CREATION
> src/slave/flags.hpp f1b8dfbb7391167b67a9498561742aa9ab9089a6
> src/slave/main.cpp 2ff2b0d186dd63e1437956e98e3682d83f2f5cd6
> src/slave/slave.cpp b234f5359a91a293b93f97f035b08ef0a4c9b20d
>
> Diff: https://reviews.apache.org/r/28656/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 28656: Added environment decorator for Slave.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28656/#review68002
-----------------------------------------------------------
Ship it!
Ship It!
- Niklas Nielsen
On Jan. 13, 2015, 5:31 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28656/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2015, 5:31 p.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2060
> https://issues.apache.org/jira/browse/MESOS-2060
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Similar to label decorator for Master.
>
>
> Diffs
> -----
>
> src/hook/hook.hpp PRE-CREATION
> src/hook/manager.hpp PRE-CREATION
> src/hook/manager.cpp PRE-CREATION
> src/slave/flags.hpp f1b8dfbb7391167b67a9498561742aa9ab9089a6
> src/slave/main.cpp 2ff2b0d186dd63e1437956e98e3682d83f2f5cd6
> src/slave/slave.cpp b234f5359a91a293b93f97f035b08ef0a4c9b20d
>
> Diff: https://reviews.apache.org/r/28656/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 28656: Added environment decorator for Slave.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28656/#review75562
-----------------------------------------------------------
src/slave/slave.cpp
<https://reviews.apache.org/r/28656/#comment122747>
Whoops! Mutating the ExecutorInfo means the master sees a mutated version when it fails over and receives a re-registration message from the slave.
This means that subsequent task launches for the executor are going to fail because the ExecutorInfo comparison fails.
This will block 0.22.0:
https://issues.apache.org/jira/browse/MESOS-2463
- Ben Mahler
On Jan. 14, 2015, 1:31 a.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28656/
> -----------------------------------------------------------
>
> (Updated Jan. 14, 2015, 1:31 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2060
> https://issues.apache.org/jira/browse/MESOS-2060
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Similar to label decorator for Master.
>
>
> Diffs
> -----
>
> src/hook/hook.hpp PRE-CREATION
> src/hook/manager.hpp PRE-CREATION
> src/hook/manager.cpp PRE-CREATION
> src/slave/flags.hpp f1b8dfbb7391167b67a9498561742aa9ab9089a6
> src/slave/main.cpp 2ff2b0d186dd63e1437956e98e3682d83f2f5cd6
> src/slave/slave.cpp b234f5359a91a293b93f97f035b08ef0a4c9b20d
>
> Diff: https://reviews.apache.org/r/28656/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 28656: Added environment decorator for Slave.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28656/
-----------------------------------------------------------
(Updated Jan. 13, 2015, 8:31 p.m.)
Review request for mesos and Niklas Nielsen.
Changes
-------
Made helper function static.
Bugs: MESOS-2060
https://issues.apache.org/jira/browse/MESOS-2060
Repository: mesos-git
Description
-------
Similar to label decorator for Master.
Diffs (updated)
-----
src/hook/hook.hpp PRE-CREATION
src/hook/manager.hpp PRE-CREATION
src/hook/manager.cpp PRE-CREATION
src/slave/flags.hpp f1b8dfbb7391167b67a9498561742aa9ab9089a6
src/slave/main.cpp 2ff2b0d186dd63e1437956e98e3682d83f2f5cd6
src/slave/slave.cpp b234f5359a91a293b93f97f035b08ef0a4c9b20d
Diff: https://reviews.apache.org/r/28656/diff/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 28656: Added environment decorator for Slave.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28656/
-----------------------------------------------------------
(Updated Jan. 13, 2015, 8:05 p.m.)
Review request for mesos and Niklas Nielsen.
Changes
-------
minor fixes.
Bugs: MESOS-2060
https://issues.apache.org/jira/browse/MESOS-2060
Repository: mesos-git
Description
-------
Similar to label decorator for Master.
Diffs (updated)
-----
src/hook/hook.hpp PRE-CREATION
src/hook/manager.hpp PRE-CREATION
src/hook/manager.cpp PRE-CREATION
src/slave/flags.hpp f1b8dfbb7391167b67a9498561742aa9ab9089a6
src/slave/main.cpp 2ff2b0d186dd63e1437956e98e3682d83f2f5cd6
src/slave/slave.hpp 70bd8c1fde4ea09fa54c76aa93424a1adb0309f6
src/slave/slave.cpp b234f5359a91a293b93f97f035b08ef0a4c9b20d
Diff: https://reviews.apache.org/r/28656/diff/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 28656: Added environment decorator for Slave.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28656/
-----------------------------------------------------------
(Updated Jan. 13, 2015, 7:46 p.m.)
Review request for mesos and Niklas Nielsen.
Bugs: MESOS-2060
https://issues.apache.org/jira/browse/MESOS-2060
Repository: mesos-git
Description
-------
Similar to label decorator for Master.
Diffs (updated)
-----
src/hook/hook.hpp PRE-CREATION
src/hook/manager.hpp PRE-CREATION
src/hook/manager.cpp PRE-CREATION
src/slave/flags.hpp f1b8dfbb7391167b67a9498561742aa9ab9089a6
src/slave/main.cpp 2ff2b0d186dd63e1437956e98e3682d83f2f5cd6
src/slave/slave.hpp 70bd8c1fde4ea09fa54c76aa93424a1adb0309f6
src/slave/slave.cpp b234f5359a91a293b93f97f035b08ef0a4c9b20d
Diff: https://reviews.apache.org/r/28656/diff/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 28656: Added environment decorator for Slave.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28656/
-----------------------------------------------------------
(Updated Jan. 13, 2015, 7:36 p.m.)
Review request for mesos and Niklas Nielsen.
Changes
-------
Addressed Nik's comments.
Bugs: MESOS-2060
https://issues.apache.org/jira/browse/MESOS-2060
Repository: mesos-git
Description
-------
Similar to label decorator for Master.
Diffs (updated)
-----
src/hook/hook.hpp PRE-CREATION
src/hook/manager.hpp PRE-CREATION
src/hook/manager.cpp PRE-CREATION
src/slave/flags.hpp f1b8dfbb7391167b67a9498561742aa9ab9089a6
src/slave/main.cpp 2ff2b0d186dd63e1437956e98e3682d83f2f5cd6
src/slave/slave.hpp 70bd8c1fde4ea09fa54c76aa93424a1adb0309f6
src/slave/slave.cpp b234f5359a91a293b93f97f035b08ef0a4c9b20d
Diff: https://reviews.apache.org/r/28656/diff/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 28656: Added environment decorator for Slave.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28656/
-----------------------------------------------------------
(Updated Jan. 13, 2015, 1:24 p.m.)
Review request for mesos and Niklas Nielsen.
Changes
-------
addressed nik's comments.
Bugs: MESOS-2060
https://issues.apache.org/jira/browse/MESOS-2060
Repository: mesos-git
Description
-------
Similar to label decorator for Master.
Diffs (updated)
-----
src/hook/hook.hpp PRE-CREATION
src/hook/manager.hpp PRE-CREATION
src/hook/manager.cpp PRE-CREATION
src/slave/flags.hpp f1b8dfbb7391167b67a9498561742aa9ab9089a6
src/slave/main.cpp 2ff2b0d186dd63e1437956e98e3682d83f2f5cd6
src/slave/slave.hpp 70bd8c1fde4ea09fa54c76aa93424a1adb0309f6
src/slave/slave.cpp b234f5359a91a293b93f97f035b08ef0a4c9b20d
Diff: https://reviews.apache.org/r/28656/diff/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 28656: Added environment decorator for Slave.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28656/
-----------------------------------------------------------
(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
-------
Similar to label decorator for Master.
Diffs
-----
src/hook/hook.hpp PRE-CREATION
src/hook/manager.hpp PRE-CREATION
src/hook/manager.cpp PRE-CREATION
src/slave/flags.hpp 670997dc3a702cd5edf33f2e5824c5e4dfe4ecef
src/slave/main.cpp 2ff2b0d186dd63e1437956e98e3682d83f2f5cd6
src/slave/slave.hpp 70bd8c1fde4ea09fa54c76aa93424a1adb0309f6
src/slave/slave.cpp 50b57819b55bdcdb9f49f20648199badc4d3f37b
Diff: https://reviews.apache.org/r/28656/diff/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 28656: Added environment decorator for Slave.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28656/
-----------------------------------------------------------
(Updated Dec. 30, 2014, 6:57 p.m.)
Review request for mesos and Niklas Nielsen.
Changes
-------
Rebased.
Repository: mesos-git
Description
-------
Similar to label decorator for Master.
Diffs (updated)
-----
src/hook/hook.hpp PRE-CREATION
src/hook/manager.hpp PRE-CREATION
src/hook/manager.cpp PRE-CREATION
src/slave/flags.hpp 670997dc3a702cd5edf33f2e5824c5e4dfe4ecef
src/slave/main.cpp 2ff2b0d186dd63e1437956e98e3682d83f2f5cd6
src/slave/slave.hpp 70bd8c1fde4ea09fa54c76aa93424a1adb0309f6
src/slave/slave.cpp 50b57819b55bdcdb9f49f20648199badc4d3f37b
Diff: https://reviews.apache.org/r/28656/diff/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 28656: Added environment decorator for Slave.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28656/
-----------------------------------------------------------
(Updated Dec. 17, 2014, 7:37 p.m.)
Review request for mesos and Niklas Nielsen.
Changes
-------
Addressed Till's comment.
Repository: mesos-git
Description
-------
Similar to label decorator for Master.
Diffs (updated)
-----
src/hook/hook.hpp PRE-CREATION
src/hook/manager.hpp PRE-CREATION
src/hook/manager.cpp PRE-CREATION
src/slave/flags.hpp 670997dc3a702cd5edf33f2e5824c5e4dfe4ecef
src/slave/main.cpp 087944aab56685b53772491de39b373ecacd6c57
src/slave/slave.hpp 70bd8c1fde4ea09fa54c76aa93424a1adb0309f6
src/slave/slave.cpp 50b57819b55bdcdb9f49f20648199badc4d3f37b
Diff: https://reviews.apache.org/r/28656/diff/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 28656: Added environment decorator for Slave.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28656/#review65392
-----------------------------------------------------------
src/slave/flags.hpp
<https://reviews.apache.org/r/28656/#comment108496>
See task label decorator RR comment.
- Till Toenshoff
On Dec. 9, 2014, 10:21 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28656/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2014, 10:21 p.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Similar to label decorator for Master.
>
>
> Diffs
> -----
>
> src/hook/hook.hpp PRE-CREATION
> src/hook/manager.hpp PRE-CREATION
> src/hook/manager.cpp PRE-CREATION
> src/slave/flags.hpp 670997dc3a702cd5edf33f2e5824c5e4dfe4ecef
> src/slave/main.cpp 087944aab56685b53772491de39b373ecacd6c57
> src/slave/slave.hpp 70bd8c1fde4ea09fa54c76aa93424a1adb0309f6
> src/slave/slave.cpp 50b57819b55bdcdb9f49f20648199badc4d3f37b
>
> Diff: https://reviews.apache.org/r/28656/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 28656: Added environment decorator for Slave.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28656/
-----------------------------------------------------------
(Updated Dec. 9, 2014, 5:21 p.m.)
Review request for mesos and Niklas Nielsen.
Changes
-------
Addressed Nik's comments; created a different RR for removeExecutorHook.
Summary (updated)
-----------------
Added environment decorator for Slave.
Repository: mesos-git
Description (updated)
-------
Similar to label decorator for Master.
Diffs (updated)
-----
src/hook/hook.hpp PRE-CREATION
src/hook/manager.hpp PRE-CREATION
src/hook/manager.cpp PRE-CREATION
src/slave/flags.hpp 670997dc3a702cd5edf33f2e5824c5e4dfe4ecef
src/slave/main.cpp 087944aab56685b53772491de39b373ecacd6c57
src/slave/slave.hpp 70bd8c1fde4ea09fa54c76aa93424a1adb0309f6
src/slave/slave.cpp 50b57819b55bdcdb9f49f20648199badc4d3f37b
Diff: https://reviews.apache.org/r/28656/diff/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 28656: Added environment decorator for Slave.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On Dec. 5, 2014, 6:01 p.m., Niklas Nielsen wrote:
> > src/hook/hook.hpp, lines 36-38
> > <https://reviews.apache.org/r/28656/diff/1/?file=781781#file781781line36>
> >
> > We need to separate these into task label vs environment decorator ones.
Added https://reviews.apache.org/r/28875/
> On Dec. 5, 2014, 6:01 p.m., Niklas Nielsen wrote:
> > src/slave/slave.cpp, line 3836
> > <https://reviews.apache.org/r/28656/diff/1/?file=781787#file781787line3836>
> >
> > Why do we need this split? I know that we talked about this, if we used the hooks object to do both pre and post actions. We are not doing that anymore, so I would try not to make that split.
The split is good as we can retain the "const"ness of the incoming arguments.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28656/#review64106
-----------------------------------------------------------
On Dec. 9, 2014, 5:21 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28656/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2014, 5:21 p.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Similar to label decorator for Master.
>
>
> Diffs
> -----
>
> src/hook/hook.hpp PRE-CREATION
> src/hook/manager.hpp PRE-CREATION
> src/hook/manager.cpp PRE-CREATION
> src/slave/flags.hpp 670997dc3a702cd5edf33f2e5824c5e4dfe4ecef
> src/slave/main.cpp 087944aab56685b53772491de39b373ecacd6c57
> src/slave/slave.hpp 70bd8c1fde4ea09fa54c76aa93424a1adb0309f6
> src/slave/slave.cpp 50b57819b55bdcdb9f49f20648199badc4d3f37b
>
> Diff: https://reviews.apache.org/r/28656/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 28656: Added Slave hooks.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28656/#review64106
-----------------------------------------------------------
You need to rebase both. That should also give you the Labels Message.
General comment - let's split the task label and environment decorators into their own reviews.
We need to think carefully about where to do the decorator placement and how decorators can _remove_ fields.
As you mentioned in the previous RR; we need tests to go along with it.
src/hook/hook.hpp
<https://reviews.apache.org/r/28656/#comment106541>
We need to separate these into task label vs environment decorator ones.
src/hook/manager.hpp
<https://reviews.apache.org/r/28656/#comment106547>
Please disregard my other comment on the whole enum, but I am still wondering what you are using NUM_HOOKS for?
src/hook/manager.cpp
<https://reviews.apache.org/r/28656/#comment106543>
s/newEnvironment/result/g?
src/slave/slave.cpp
<https://reviews.apache.org/r/28656/#comment106544>
Can we move this further down - maybe as one of the last statements?
src/slave/slave.cpp
<https://reviews.apache.org/r/28656/#comment106546>
Why do we need this split? I know that we talked about this, if we used the hooks object to do both pre and post actions. We are not doing that anymore, so I would try not to make that split.
- Niklas Nielsen
On Dec. 3, 2014, 10:15 a.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28656/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2014, 10:15 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Similar to Master hooks.
>
>
> Diffs
> -----
>
> src/hook/hook.hpp PRE-CREATION
> src/hook/manager.hpp PRE-CREATION
> src/hook/manager.cpp PRE-CREATION
> src/slave/flags.hpp fee79e04cca14e136241af06af1fa9105a70b6dd
> src/slave/main.cpp 087944aab56685b53772491de39b373ecacd6c57
> src/slave/slave.hpp c6d11ef2de77738ab46d7ca02778f7237d230a5c
> src/slave/slave.cpp 373c8b44f5bc000da801b0925e1a9c346d9ffabf
>
> Diff: https://reviews.apache.org/r/28656/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>