Re: Review Request 30339: Call hookmanager only if some hooks are installed.
--- 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.
---
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.
--- 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.
--- 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.
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.
--- 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.
--- 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.
--- 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.
--- 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.
--- 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.
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
