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/28 01:42:06 UTC
Review Request 30339: Use flags.hooks.isSome() before calling hooks.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30339/
-----------------------------------------------------------
Review request for mesos, Niklas Nielsen and Till Toenshoff.
Repository: mesos-git
Description
-------
Call hook manager only if hooks were specified on the commandline.
Diffs
-----
src/master/master.cpp bda8fda9bc2e52ccc1d75e2541e4604989515e13
src/slave/slave.hpp 70bd8c1fde4ea09fa54c76aa93424a1adb0309f6
src/slave/slave.cpp fca83b3977b95ddda30f9830da10e124b5c605e6
src/tests/hook_tests.cpp 44f73effdce2d03627215418007ccbc3263a0c52
Diff: https://reviews.apache.org/r/30339/diff/
Testing
-------
make check
Thanks,
Kapil Arya
Re: Review Request 30339: Use flags.hooks.isSome() before calling
hooks.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On March 4, 2015, 4:03 p.m., Niklas Nielsen wrote:
> >
Do you want this in? If so, please update the review :)
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30339/#review72824
-----------------------------------------------------------
On Jan. 27, 2015, 4:42 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30339/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2015, 4:42 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Call hook manager only if hooks were specified on the commandline.
>
>
> Diffs
> -----
>
> src/master/master.cpp bda8fda9bc2e52ccc1d75e2541e4604989515e13
> src/slave/slave.hpp 70bd8c1fde4ea09fa54c76aa93424a1adb0309f6
> src/slave/slave.cpp fca83b3977b95ddda30f9830da10e124b5c605e6
> src/tests/hook_tests.cpp 44f73effdce2d03627215418007ccbc3263a0c52
>
> Diff: https://reviews.apache.org/r/30339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 30339: Use flags.hooks.isSome() before calling
hooks.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On March 4, 2015, 4:03 p.m., Niklas Nielsen wrote:
> >
>
> Niklas Nielsen wrote:
> Do you want this in? If so, please update the review :)
Ping - want this in? :)
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30339/#review72824
-----------------------------------------------------------
On Jan. 27, 2015, 4:42 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30339/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2015, 4:42 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Call hook manager only if hooks were specified on the commandline.
>
>
> Diffs
> -----
>
> src/master/master.cpp bda8fda9bc2e52ccc1d75e2541e4604989515e13
> src/slave/slave.hpp 70bd8c1fde4ea09fa54c76aa93424a1adb0309f6
> src/slave/slave.cpp fca83b3977b95ddda30f9830da10e124b5c605e6
> src/tests/hook_tests.cpp 44f73effdce2d03627215418007ccbc3263a0c52
>
> Diff: https://reviews.apache.org/r/30339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 30339: Call hookmanager only if some hooks are
installed.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On March 4, 2015, 7:03 p.m., Niklas Nielsen wrote:
> > src/tests/hook_tests.cpp, line 229
> > <https://reviews.apache.org/r/30339/diff/1/?file=837755#file837755line229>
> >
> > Where does this constant come from? Should we perhaps move this into CreateMasterFlags()?
This is required for hooks only. The const is defined above in the file. It corresponds to the name of the module.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30339/#review72824
-----------------------------------------------------------
On June 1, 2015, 5:26 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30339/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 5:26 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Call hook manager only if hooks were specified on the commandline.
>
>
> Diffs
> -----
>
> src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d
> src/hook/manager.cpp 54b0d34b6e9f2f8a8cf7a6c2f5ded2f6ab6c6955
> src/master/master.cpp 710b8149c9d855d0f47cb2952366be10bc78c74d
> src/slave/containerizer/containerizer.cpp 4d66e767de1f877cb66b37826ba7c9d00639a7c0
> src/slave/slave.cpp 271cb03770cd08406054dfce35d0821475e49b05
> src/tests/hook_tests.cpp 3ffde6d6b2faeb5a8a40eb27c3b0a2b7f9ecd2b1
>
> Diff: https://reviews.apache.org/r/30339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 30339: Use flags.hooks.isSome() before calling
hooks.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30339/#review72824
-----------------------------------------------------------
src/slave/slave.hpp
<https://reviews.apache.org/r/30339/#comment118916>
Can you split this into it's own commit or document it in the review request description?
src/slave/slave.cpp
<https://reviews.apache.org/r/30339/#comment122235>
Needs 2 space indent extra.
src/tests/hook_tests.cpp
<https://reviews.apache.org/r/30339/#comment122232>
s/> >/>>/g
src/tests/hook_tests.cpp
<https://reviews.apache.org/r/30339/#comment122233>
Where does this constant come from? Should we perhaps move this into CreateMasterFlags()?
src/tests/hook_tests.cpp
<https://reviews.apache.org/r/30339/#comment122234>
Same here.
- Niklas Nielsen
On Jan. 27, 2015, 4:42 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30339/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2015, 4:42 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Call hook manager only if hooks were specified on the commandline.
>
>
> Diffs
> -----
>
> src/master/master.cpp bda8fda9bc2e52ccc1d75e2541e4604989515e13
> src/slave/slave.hpp 70bd8c1fde4ea09fa54c76aa93424a1adb0309f6
> src/slave/slave.cpp fca83b3977b95ddda30f9830da10e124b5c605e6
> src/tests/hook_tests.cpp 44f73effdce2d03627215418007ccbc3263a0c52
>
> Diff: https://reviews.apache.org/r/30339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 30339: Use flags.hooks.isSome() before calling
hooks.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30339/#review69933
-----------------------------------------------------------
Patch looks great!
Reviews applied: [29947, 30339]
All tests passed.
- Mesos ReviewBot
On Jan. 28, 2015, 12:42 a.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30339/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2015, 12:42 a.m.)
>
>
> Review request for mesos, Niklas Nielsen and Till Toenshoff.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Call hook manager only if hooks were specified on the commandline.
>
>
> Diffs
> -----
>
> src/master/master.cpp bda8fda9bc2e52ccc1d75e2541e4604989515e13
> src/slave/slave.hpp 70bd8c1fde4ea09fa54c76aa93424a1adb0309f6
> src/slave/slave.cpp fca83b3977b95ddda30f9830da10e124b5c605e6
> src/tests/hook_tests.cpp 44f73effdce2d03627215418007ccbc3263a0c52
>
> Diff: https://reviews.apache.org/r/30339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 30339: Call hookmanager only if some hooks are
installed.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On June 1, 2015, 5:41 p.m., Niklas Nielsen wrote:
> > Do you have a JIRA ticket for this change?
> >
> > Is it to avoid memory copies, that you need these to be in the call sites. It clutters the call sites a bit, so I wanted to seee if we could move this into the HooksManager instead.
> >
> > Could we wrap the hook calls in a macro or a helper taking a lambda, which could simplify the call sites?
I don't think there is a JIRA ticket for this change. It was a suggestion by vinod or Ben.
Creating macros around it would be unclean. In certain cases, we are relying on the return value to update some fields and putting macros would just make it ugly. To me this current form seems more readable and makes it clear to the reader that the code would be called only if there are some hooks installed.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30339/#review86091
-----------------------------------------------------------
On June 1, 2015, 5:46 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30339/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 5:46 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Call hook manager only if hooks were specified on the commandline.
>
>
> Diffs
> -----
>
> src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d
> src/hook/manager.cpp 54b0d34b6e9f2f8a8cf7a6c2f5ded2f6ab6c6955
> src/master/master.cpp 710b8149c9d855d0f47cb2952366be10bc78c74d
> src/slave/containerizer/containerizer.cpp 4d66e767de1f877cb66b37826ba7c9d00639a7c0
> src/slave/slave.cpp 271cb03770cd08406054dfce35d0821475e49b05
> src/tests/hook_tests.cpp 3ffde6d6b2faeb5a8a40eb27c3b0a2b7f9ecd2b1
>
> Diff: https://reviews.apache.org/r/30339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 30339: Call hookmanager only if some hooks are
installed.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On June 1, 2015, 2:41 p.m., Niklas Nielsen wrote:
> > Do you have a JIRA ticket for this change?
> >
> > Is it to avoid memory copies, that you need these to be in the call sites. It clutters the call sites a bit, so I wanted to seee if we could move this into the HooksManager instead.
> >
> > Could we wrap the hook calls in a macro or a helper taking a lambda, which could simplify the call sites?
>
> Kapil Arya wrote:
> I don't think there is a JIRA ticket for this change. It was a suggestion by vinod or Ben.
>
> Creating macros around it would be unclean. In certain cases, we are relying on the return value to update some fields and putting macros would just make it ugly. To me this current form seems more readable and makes it clear to the reader that the code would be called only if there are some hooks installed.
Would you mind creating a JIRA for it then? :)
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30339/#review86091
-----------------------------------------------------------
On June 1, 2015, 2:46 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30339/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 2:46 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Call hook manager only if hooks were specified on the commandline.
>
>
> Diffs
> -----
>
> src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d
> src/hook/manager.cpp 54b0d34b6e9f2f8a8cf7a6c2f5ded2f6ab6c6955
> src/master/master.cpp 710b8149c9d855d0f47cb2952366be10bc78c74d
> src/slave/containerizer/containerizer.cpp 4d66e767de1f877cb66b37826ba7c9d00639a7c0
> src/slave/slave.cpp 271cb03770cd08406054dfce35d0821475e49b05
> src/tests/hook_tests.cpp 3ffde6d6b2faeb5a8a40eb27c3b0a2b7f9ecd2b1
>
> Diff: https://reviews.apache.org/r/30339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 30339: Call hookmanager only if some hooks are
installed.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30339/#review86091
-----------------------------------------------------------
Do you have a JIRA ticket for this change?
Is it to avoid memory copies, that you need these to be in the call sites. It clutters the call sites a bit, so I wanted to seee if we could move this into the HooksManager instead.
Could we wrap the hook calls in a macro or a helper taking a lambda, which could simplify the call sites?
src/hook/manager.cpp
<https://reviews.apache.org/r/30339/#comment137987>
is .empty() available?
- Niklas Nielsen
On June 1, 2015, 2:26 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30339/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 2:26 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Call hook manager only if hooks were specified on the commandline.
>
>
> Diffs
> -----
>
> src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d
> src/hook/manager.cpp 54b0d34b6e9f2f8a8cf7a6c2f5ded2f6ab6c6955
> src/master/master.cpp 710b8149c9d855d0f47cb2952366be10bc78c74d
> src/slave/containerizer/containerizer.cpp 4d66e767de1f877cb66b37826ba7c9d00639a7c0
> src/slave/slave.cpp 271cb03770cd08406054dfce35d0821475e49b05
> src/tests/hook_tests.cpp 3ffde6d6b2faeb5a8a40eb27c3b0a2b7f9ecd2b1
>
> Diff: https://reviews.apache.org/r/30339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 30339: Call hookmanager only if some hooks are
installed.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30339/#review88088
-----------------------------------------------------------
Ship it!
Ship It!
- Niklas Nielsen
On June 15, 2015, 3:26 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30339/
> -----------------------------------------------------------
>
> (Updated June 15, 2015, 3:26 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Call hook manager only if hooks were specified on the commandline.
>
>
> Diffs
> -----
>
> src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d
> src/hook/manager.cpp b43b91878cc9f0f5f5d081b9b3af0768c07c282b
> src/master/master.cpp 0137c37a42d4f7a9057568a8cd7b662ed92be353
> src/slave/containerizer/containerizer.cpp e995ce602261c18373ac09c823638c4a252cca86
> src/slave/slave.cpp 67732a40ef683cb0188425f0bba8fe8ab83e461c
>
> Diff: https://reviews.apache.org/r/30339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 30339: Call hookmanager only if some hooks are
installed.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30339/#review87991
-----------------------------------------------------------
Patch looks great!
Reviews applied: [30339]
All tests passed.
- Mesos ReviewBot
On June 15, 2015, 10:26 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30339/
> -----------------------------------------------------------
>
> (Updated June 15, 2015, 10:26 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Call hook manager only if hooks were specified on the commandline.
>
>
> Diffs
> -----
>
> src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d
> src/hook/manager.cpp b43b91878cc9f0f5f5d081b9b3af0768c07c282b
> src/master/master.cpp 0137c37a42d4f7a9057568a8cd7b662ed92be353
> src/slave/containerizer/containerizer.cpp e995ce602261c18373ac09c823638c4a252cca86
> src/slave/slave.cpp 67732a40ef683cb0188425f0bba8fe8ab83e461c
>
> Diff: https://reviews.apache.org/r/30339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 30339: Call hookmanager only if some hooks are
installed.
Posted by Joris Van Remoortere <jo...@gmail.com>.
> On June 16, 2015, 4:55 p.m., Niklas Nielsen wrote:
> > src/hook/manager.cpp, lines 96-98
> > <https://reviews.apache.org/r/30339/diff/5/?file=984855#file984855line96>
> >
> > Won't some compilers potentially break with reaching end of non-void function?
> >
> > How about:
> >
> > ```
> > bool result = false;
> >
> > synchronized(mutex) {
> > return !availableHooks.empty();
> > }
> >
> > return result;
> > ```
> >
> > @joris - any insights on what is safe to do here?
After mpark's changes to synchronized, it is now safe to do this :-) The key is informing the compiler that this path is always executed (which is what mpark's refactoring does to the synchronized macro).
- Joris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30339/#review88085
-----------------------------------------------------------
On June 15, 2015, 10:26 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30339/
> -----------------------------------------------------------
>
> (Updated June 15, 2015, 10:26 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Call hook manager only if hooks were specified on the commandline.
>
>
> Diffs
> -----
>
> src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d
> src/hook/manager.cpp b43b91878cc9f0f5f5d081b9b3af0768c07c282b
> src/master/master.cpp 0137c37a42d4f7a9057568a8cd7b662ed92be353
> src/slave/containerizer/containerizer.cpp e995ce602261c18373ac09c823638c4a252cca86
> src/slave/slave.cpp 67732a40ef683cb0188425f0bba8fe8ab83e461c
>
> Diff: https://reviews.apache.org/r/30339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 30339: Call hookmanager only if some hooks are
installed.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30339/#review88085
-----------------------------------------------------------
src/hook/manager.cpp (lines 96 - 98)
<https://reviews.apache.org/r/30339/#comment140488>
Won't some compilers potentially break with reaching end of non-void function?
How about:
```
bool result = false;
synchronized(mutex) {
return !availableHooks.empty();
}
return result;
```
@joris - any insights on what is safe to do here?
- Niklas Nielsen
On June 15, 2015, 3:26 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30339/
> -----------------------------------------------------------
>
> (Updated June 15, 2015, 3:26 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Call hook manager only if hooks were specified on the commandline.
>
>
> Diffs
> -----
>
> src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d
> src/hook/manager.cpp b43b91878cc9f0f5f5d081b9b3af0768c07c282b
> src/master/master.cpp 0137c37a42d4f7a9057568a8cd7b662ed92be353
> src/slave/containerizer/containerizer.cpp e995ce602261c18373ac09c823638c4a252cca86
> src/slave/slave.cpp 67732a40ef683cb0188425f0bba8fe8ab83e461c
>
> Diff: https://reviews.apache.org/r/30339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 30339: Call hookmanager only if some hooks are
installed.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30339/
-----------------------------------------------------------
(Updated June 17, 2015, 5:06 p.m.)
Review request for mesos, Niklas Nielsen and Till Toenshoff.
Changes
-------
Added Jira issue
Bugs: MESOS-2883
https://issues.apache.org/jira/browse/MESOS-2883
Repository: mesos
Description
-------
Call hook manager only if hooks were specified on the commandline.
Diffs
-----
src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d
src/hook/manager.cpp b43b91878cc9f0f5f5d081b9b3af0768c07c282b
src/master/master.cpp 0137c37a42d4f7a9057568a8cd7b662ed92be353
src/slave/containerizer/containerizer.cpp e995ce602261c18373ac09c823638c4a252cca86
src/slave/slave.cpp 67732a40ef683cb0188425f0bba8fe8ab83e461c
Diff: https://reviews.apache.org/r/30339/diff/
Testing
-------
make check
Thanks,
Kapil Arya
Re: Review Request 30339: Call hookmanager only if some hooks are
installed.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30339/
-----------------------------------------------------------
(Updated June 15, 2015, 6:26 p.m.)
Review request for mesos, Niklas Nielsen and Till Toenshoff.
Changes
-------
Removed unneeded changes from hook tests.
Repository: mesos
Description
-------
Call hook manager only if hooks were specified on the commandline.
Diffs (updated)
-----
src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d
src/hook/manager.cpp b43b91878cc9f0f5f5d081b9b3af0768c07c282b
src/master/master.cpp 0137c37a42d4f7a9057568a8cd7b662ed92be353
src/slave/containerizer/containerizer.cpp e995ce602261c18373ac09c823638c4a252cca86
src/slave/slave.cpp 67732a40ef683cb0188425f0bba8fe8ab83e461c
Diff: https://reviews.apache.org/r/30339/diff/
Testing
-------
make check
Thanks,
Kapil Arya
Re: Review Request 30339: Call hookmanager only if some hooks are
installed.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30339/
-----------------------------------------------------------
(Updated June 15, 2015, 5:22 p.m.)
Review request for mesos, Niklas Nielsen and Till Toenshoff.
Changes
-------
Added mutex check
Repository: mesos
Description
-------
Call hook manager only if hooks were specified on the commandline.
Diffs (updated)
-----
src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d
src/hook/manager.cpp 54b0d34b6e9f2f8a8cf7a6c2f5ded2f6ab6c6955
src/master/master.cpp 710b8149c9d855d0f47cb2952366be10bc78c74d
src/slave/containerizer/containerizer.cpp 4d66e767de1f877cb66b37826ba7c9d00639a7c0
src/slave/slave.cpp 271cb03770cd08406054dfce35d0821475e49b05
src/tests/hook_tests.cpp 3ffde6d6b2faeb5a8a40eb27c3b0a2b7f9ecd2b1
Diff: https://reviews.apache.org/r/30339/diff/
Testing
-------
make check
Thanks,
Kapil Arya
Re: Review Request 30339: Call hookmanager only if some hooks are
installed.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On June 4, 2015, 3:39 a.m., Till Toenshoff wrote:
> > src/tests/hook_tests.cpp, line 132
> > <https://reviews.apache.org/r/30339/diff/3/?file=975944#file975944line132>
> >
> > Could you please explain quickly on why this was ever working / is needed now?
Good point. As it turns out, we don't need these changes anymore since we explicitly do `HookManager::initialize(HOOK_MODULE_NAME)`. This forces the hooksAvailable() to always return true. Thanks for the catch!
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30339/#review86574
-----------------------------------------------------------
On June 15, 2015, 6:26 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30339/
> -----------------------------------------------------------
>
> (Updated June 15, 2015, 6:26 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Call hook manager only if hooks were specified on the commandline.
>
>
> Diffs
> -----
>
> src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d
> src/hook/manager.cpp b43b91878cc9f0f5f5d081b9b3af0768c07c282b
> src/master/master.cpp 0137c37a42d4f7a9057568a8cd7b662ed92be353
> src/slave/containerizer/containerizer.cpp e995ce602261c18373ac09c823638c4a252cca86
> src/slave/slave.cpp 67732a40ef683cb0188425f0bba8fe8ab83e461c
>
> Diff: https://reviews.apache.org/r/30339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 30339: Call hookmanager only if some hooks are
installed.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30339/#review86574
-----------------------------------------------------------
src/hook/manager.cpp
<https://reviews.apache.org/r/30339/#comment138629>
+1!
src/tests/hook_tests.cpp
<https://reviews.apache.org/r/30339/#comment138628>
Could you please explain quickly on why this was ever working / is needed now?
- Till Toenshoff
On June 1, 2015, 9:46 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30339/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 9:46 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Call hook manager only if hooks were specified on the commandline.
>
>
> Diffs
> -----
>
> src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d
> src/hook/manager.cpp 54b0d34b6e9f2f8a8cf7a6c2f5ded2f6ab6c6955
> src/master/master.cpp 710b8149c9d855d0f47cb2952366be10bc78c74d
> src/slave/containerizer/containerizer.cpp 4d66e767de1f877cb66b37826ba7c9d00639a7c0
> src/slave/slave.cpp 271cb03770cd08406054dfce35d0821475e49b05
> src/tests/hook_tests.cpp 3ffde6d6b2faeb5a8a40eb27c3b0a2b7f9ecd2b1
>
> Diff: https://reviews.apache.org/r/30339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 30339: Call hookmanager only if some hooks are
installed.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On June 3, 2015, 1:17 p.m., Niklas Nielsen wrote:
> > src/hook/manager.cpp, line 95
> > <https://reviews.apache.org/r/30339/diff/3/?file=975940#file975940line95>
> >
> > Don't you need to acquire the mutex here?
Good catch. Fixed.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30339/#review86436
-----------------------------------------------------------
On June 15, 2015, 5:22 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30339/
> -----------------------------------------------------------
>
> (Updated June 15, 2015, 5:22 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Call hook manager only if hooks were specified on the commandline.
>
>
> Diffs
> -----
>
> src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d
> src/hook/manager.cpp 54b0d34b6e9f2f8a8cf7a6c2f5ded2f6ab6c6955
> src/master/master.cpp 710b8149c9d855d0f47cb2952366be10bc78c74d
> src/slave/containerizer/containerizer.cpp 4d66e767de1f877cb66b37826ba7c9d00639a7c0
> src/slave/slave.cpp 271cb03770cd08406054dfce35d0821475e49b05
> src/tests/hook_tests.cpp 3ffde6d6b2faeb5a8a40eb27c3b0a2b7f9ecd2b1
>
> Diff: https://reviews.apache.org/r/30339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 30339: Call hookmanager only if some hooks are
installed.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30339/#review86436
-----------------------------------------------------------
src/hook/manager.cpp
<https://reviews.apache.org/r/30339/#comment138427>
Don't you need to acquire the mutex here?
- Niklas Nielsen
On June 1, 2015, 2:46 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30339/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 2:46 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Call hook manager only if hooks were specified on the commandline.
>
>
> Diffs
> -----
>
> src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d
> src/hook/manager.cpp 54b0d34b6e9f2f8a8cf7a6c2f5ded2f6ab6c6955
> src/master/master.cpp 710b8149c9d855d0f47cb2952366be10bc78c74d
> src/slave/containerizer/containerizer.cpp 4d66e767de1f877cb66b37826ba7c9d00639a7c0
> src/slave/slave.cpp 271cb03770cd08406054dfce35d0821475e49b05
> src/tests/hook_tests.cpp 3ffde6d6b2faeb5a8a40eb27c3b0a2b7f9ecd2b1
>
> Diff: https://reviews.apache.org/r/30339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 30339: Call hookmanager only if some hooks are
installed.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30339/#review86103
-----------------------------------------------------------
Patch looks great!
Reviews applied: [30339]
All tests passed.
- Mesos ReviewBot
On June 1, 2015, 9:46 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30339/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 9:46 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Call hook manager only if hooks were specified on the commandline.
>
>
> Diffs
> -----
>
> src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d
> src/hook/manager.cpp 54b0d34b6e9f2f8a8cf7a6c2f5ded2f6ab6c6955
> src/master/master.cpp 710b8149c9d855d0f47cb2952366be10bc78c74d
> src/slave/containerizer/containerizer.cpp 4d66e767de1f877cb66b37826ba7c9d00639a7c0
> src/slave/slave.cpp 271cb03770cd08406054dfce35d0821475e49b05
> src/tests/hook_tests.cpp 3ffde6d6b2faeb5a8a40eb27c3b0a2b7f9ecd2b1
>
> Diff: https://reviews.apache.org/r/30339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 30339: Call hookmanager only if some hooks are
installed.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30339/
-----------------------------------------------------------
(Updated June 1, 2015, 5:46 p.m.)
Review request for mesos, Niklas Nielsen and Till Toenshoff.
Changes
-------
Addressed Nik's comment.
Repository: mesos
Description
-------
Call hook manager only if hooks were specified on the commandline.
Diffs (updated)
-----
src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d
src/hook/manager.cpp 54b0d34b6e9f2f8a8cf7a6c2f5ded2f6ab6c6955
src/master/master.cpp 710b8149c9d855d0f47cb2952366be10bc78c74d
src/slave/containerizer/containerizer.cpp 4d66e767de1f877cb66b37826ba7c9d00639a7c0
src/slave/slave.cpp 271cb03770cd08406054dfce35d0821475e49b05
src/tests/hook_tests.cpp 3ffde6d6b2faeb5a8a40eb27c3b0a2b7f9ecd2b1
Diff: https://reviews.apache.org/r/30339/diff/
Testing
-------
make check
Thanks,
Kapil Arya
Re: Review Request 30339: Call hookmanager only if some hooks are
installed.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30339/
-----------------------------------------------------------
(Updated June 1, 2015, 5:26 p.m.)
Review request for mesos, Niklas Nielsen and Till Toenshoff.
Changes
-------
Rebased and udpated.
Summary (updated)
-----------------
Call hookmanager only if some hooks are installed.
Repository: mesos
Description
-------
Call hook manager only if hooks were specified on the commandline.
Diffs (updated)
-----
src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d
src/hook/manager.cpp 54b0d34b6e9f2f8a8cf7a6c2f5ded2f6ab6c6955
src/master/master.cpp 710b8149c9d855d0f47cb2952366be10bc78c74d
src/slave/containerizer/containerizer.cpp 4d66e767de1f877cb66b37826ba7c9d00639a7c0
src/slave/slave.cpp 271cb03770cd08406054dfce35d0821475e49b05
src/tests/hook_tests.cpp 3ffde6d6b2faeb5a8a40eb27c3b0a2b7f9ecd2b1
Diff: https://reviews.apache.org/r/30339/diff/
Testing
-------
make check
Thanks,
Kapil Arya
Re: Review Request 30339: Call hookmanager only if some hooks are
installed.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On Feb. 18, 2015, 2:34 a.m., Till Toenshoff wrote:
> > src/tests/hook_tests.cpp, line 239
> > <https://reviews.apache.org/r/30339/diff/1/?file=837755#file837755line239>
> >
> > Sharp bracket closing; that part somehow slipped through the recent "hooks" commits - there are more of those in these tests. Worth a small followup-patch?
Outdated.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30339/#review72915
-----------------------------------------------------------
On June 1, 2015, 5:26 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30339/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 5:26 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Call hook manager only if hooks were specified on the commandline.
>
>
> Diffs
> -----
>
> src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d
> src/hook/manager.cpp 54b0d34b6e9f2f8a8cf7a6c2f5ded2f6ab6c6955
> src/master/master.cpp 710b8149c9d855d0f47cb2952366be10bc78c74d
> src/slave/containerizer/containerizer.cpp 4d66e767de1f877cb66b37826ba7c9d00639a7c0
> src/slave/slave.cpp 271cb03770cd08406054dfce35d0821475e49b05
> src/tests/hook_tests.cpp 3ffde6d6b2faeb5a8a40eb27c3b0a2b7f9ecd2b1
>
> Diff: https://reviews.apache.org/r/30339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 30339: Use flags.hooks.isSome() before calling
hooks.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30339/#review72915
-----------------------------------------------------------
Ship it!
src/tests/hook_tests.cpp
<https://reviews.apache.org/r/30339/#comment119027>
Sharp bracket closing; that part somehow slipped through the recent "hooks" commits - there are more of those in these tests. Worth a small followup-patch?
- Till Toenshoff
On Jan. 28, 2015, 12:42 a.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30339/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2015, 12:42 a.m.)
>
>
> Review request for mesos, Niklas Nielsen and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Call hook manager only if hooks were specified on the commandline.
>
>
> Diffs
> -----
>
> src/master/master.cpp bda8fda9bc2e52ccc1d75e2541e4604989515e13
> src/slave/slave.hpp 70bd8c1fde4ea09fa54c76aa93424a1adb0309f6
> src/slave/slave.cpp fca83b3977b95ddda30f9830da10e124b5c605e6
> src/tests/hook_tests.cpp 44f73effdce2d03627215418007ccbc3263a0c52
>
> Diff: https://reviews.apache.org/r/30339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Kapil Arya
>
>