You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Niklas Nielsen <ni...@qni.dk> on 2015/03/14 00:04:05 UTC
Re: Review Request 30961: Enabled label decorator to override.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30961/
-----------------------------------------------------------
(Updated March 13, 2015, 4:04 p.m.)
Review request for mesos, Ben Mahler and Kapil Arya.
Changes
-------
Rebased
Bugs: MESOS-2351
https://issues.apache.org/jira/browse/MESOS-2351
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
include/mesos/hook.hpp 9ae8b9455a86c7a5cbf4f1d1b1ce88f2811ce35d
src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41
src/hook/manager.cpp 7a4cb09bc221af502e867cfb7fff2900b599ff1f
src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1
src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
Diff: https://reviews.apache.org/r/30961/diff/
Testing
-------
make check (with modified VerifyMasterLaunchTaskHook test)
Thanks,
Niklas Nielsen
Re: Review Request 30961: Enabled label decorator to override.
Posted by Adam B <ad...@mesosphere.io>.
> On March 30, 2015, 3:23 a.m., Adam B wrote:
> > Minor cleanup/suggestions, but otherwise good. We'll definitely need to document te module(-manager) API change of overriding the label set instead of merging. This should probably go in the upgrades doc?
>
> Niklas Nielsen wrote:
> Thanks for the review Adam!
>
> Great point. I'll create a JIRA for it.
JIRA link for the upgrades doc?
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30961/#review78189
-----------------------------------------------------------
On April 2, 2015, 2:39 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30961/
> -----------------------------------------------------------
>
> (Updated April 2, 2015, 2:39 p.m.)
>
>
> Review request for mesos, Ben Mahler and Kapil Arya.
>
>
> Bugs: MESOS-2351
> https://issues.apache.org/jira/browse/MESOS-2351
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp 9ae8b9455a86c7a5cbf4f1d1b1ce88f2811ce35d
> src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41
> src/hook/manager.cpp 7a4cb09bc221af502e867cfb7fff2900b599ff1f
> src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b
> src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
>
> Diff: https://reviews.apache.org/r/30961/diff/
>
>
> Testing
> -------
>
> make check (with modified VerifyMasterLaunchTaskHook test)
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 30961: Enabled label decorator to override.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On March 30, 2015, 3:23 a.m., Adam B wrote:
> > Minor cleanup/suggestions, but otherwise good. We'll definitely need to document te module(-manager) API change of overriding the label set instead of merging. This should probably go in the upgrades doc?
Thanks for the review Adam!
Great point. I'll create a JIRA for it.
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30961/#review78189
-----------------------------------------------------------
On April 2, 2015, 2:39 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30961/
> -----------------------------------------------------------
>
> (Updated April 2, 2015, 2:39 p.m.)
>
>
> Review request for mesos, Ben Mahler and Kapil Arya.
>
>
> Bugs: MESOS-2351
> https://issues.apache.org/jira/browse/MESOS-2351
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp 9ae8b9455a86c7a5cbf4f1d1b1ce88f2811ce35d
> src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41
> src/hook/manager.cpp 7a4cb09bc221af502e867cfb7fff2900b599ff1f
> src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b
> src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
>
> Diff: https://reviews.apache.org/r/30961/diff/
>
>
> Testing
> -------
>
> make check (with modified VerifyMasterLaunchTaskHook test)
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 30961: Enabled label decorator to override.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30961/#review78189
-----------------------------------------------------------
Minor cleanup/suggestions, but otherwise good. We'll definitely need to document te module(-manager) API change of overriding the label set instead of merging. This should probably go in the upgrades doc?
src/examples/test_hook_module.cpp
<https://reviews.apache.org/r/30961/#comment126700>
Did you mix up key and Value? Would think you'd want the MESOS_Test_Remove_Label to be the key.
src/hook/manager.cpp
<https://reviews.apache.org/r/30961/#comment126702>
Why not just return `taskInfo_.labels()`?
In fact, I don't think there's really any reason to have a `labels` pointer in this function. Just do `taskInfo_.mutable_labels()->CopyFrom(result.get());` and then it's clear throughout that you're modifying the same `taskInfo_` object.
src/tests/hook_tests.cpp
<https://reviews.apache.org/r/30961/#comment126701>
Switch Key, Value
- Adam B
On March 13, 2015, 4:04 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30961/
> -----------------------------------------------------------
>
> (Updated March 13, 2015, 4:04 p.m.)
>
>
> Review request for mesos, Ben Mahler and Kapil Arya.
>
>
> Bugs: MESOS-2351
> https://issues.apache.org/jira/browse/MESOS-2351
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp 9ae8b9455a86c7a5cbf4f1d1b1ce88f2811ce35d
> src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41
> src/hook/manager.cpp 7a4cb09bc221af502e867cfb7fff2900b599ff1f
> src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1
> src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
>
> Diff: https://reviews.apache.org/r/30961/diff/
>
>
> Testing
> -------
>
> make check (with modified VerifyMasterLaunchTaskHook test)
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 30961: Enabled label decorator to override.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30961/#review80817
-----------------------------------------------------------
Ship it!
Ship It!
- Adam B
On April 20, 2015, 1:16 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30961/
> -----------------------------------------------------------
>
> (Updated April 20, 2015, 1:16 p.m.)
>
>
> Review request for mesos, Ben Mahler and Kapil Arya.
>
>
> Bugs: MESOS-2351
> https://issues.apache.org/jira/browse/MESOS-2351
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp 9ae8b9455a86c7a5cbf4f1d1b1ce88f2811ce35d
> src/examples/test_hook_module.cpp 2f2da1c5ef85af06c7f366d38ce5b64f39d0076f
> src/hook/manager.cpp 7a4cb09bc221af502e867cfb7fff2900b599ff1f
> src/master/master.cpp e30b951eda2b3b0d5b2a80716f0b32c6bbe041bc
> src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
>
> Diff: https://reviews.apache.org/r/30961/diff/
>
>
> Testing
> -------
>
> make check (with modified VerifyMasterLaunchTaskHook test)
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 30961: Enabled label decorator to override.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30961/
-----------------------------------------------------------
(Updated April 20, 2015, 1:16 p.m.)
Review request for mesos, Ben Mahler and Kapil Arya.
Bugs: MESOS-2351
https://issues.apache.org/jira/browse/MESOS-2351
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
include/mesos/hook.hpp 9ae8b9455a86c7a5cbf4f1d1b1ce88f2811ce35d
src/examples/test_hook_module.cpp 2f2da1c5ef85af06c7f366d38ce5b64f39d0076f
src/hook/manager.cpp 7a4cb09bc221af502e867cfb7fff2900b599ff1f
src/master/master.cpp e30b951eda2b3b0d5b2a80716f0b32c6bbe041bc
src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
Diff: https://reviews.apache.org/r/30961/diff/
Testing
-------
make check (with modified VerifyMasterLaunchTaskHook test)
Thanks,
Niklas Nielsen
Re: Review Request 30961: Enabled label decorator to override.
Posted by Adam B <ad...@mesosphere.io>.
> On April 8, 2015, 5:31 p.m., Adam B wrote:
> > src/examples/test_hook_module.cpp, line 36
> > <https://reviews.apache.org/r/30961/diff/7/?file=920371#file920371line36>
> >
> > Unused? Or should you check that the value being removed is what you expect?
>
> Niklas Nielsen wrote:
> It was just to keep the module and test code in sync. Would you prefer I remove it?
Yeah, when it comes to variables and constants, I say use it or lose it.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30961/#review79454
-----------------------------------------------------------
On April 7, 2015, 5:57 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30961/
> -----------------------------------------------------------
>
> (Updated April 7, 2015, 5:57 p.m.)
>
>
> Review request for mesos, Ben Mahler and Kapil Arya.
>
>
> Bugs: MESOS-2351
> https://issues.apache.org/jira/browse/MESOS-2351
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp 9ae8b9455a86c7a5cbf4f1d1b1ce88f2811ce35d
> src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41
> src/hook/manager.cpp 7a4cb09bc221af502e867cfb7fff2900b599ff1f
> src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b
> src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
>
> Diff: https://reviews.apache.org/r/30961/diff/
>
>
> Testing
> -------
>
> make check (with modified VerifyMasterLaunchTaskHook test)
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 30961: Enabled label decorator to override.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On April 8, 2015, 5:31 p.m., Adam B wrote:
> > src/examples/test_hook_module.cpp, line 36
> > <https://reviews.apache.org/r/30961/diff/7/?file=920371#file920371line36>
> >
> > Unused? Or should you check that the value being removed is what you expect?
It was just to keep the module and test code in sync. Would you prefer I remove it?
> On April 8, 2015, 5:31 p.m., Adam B wrote:
> > src/examples/test_hook_module.cpp, lines 52-57
> > <https://reviews.apache.org/r/30961/diff/7/?file=920371#file920371line52>
> >
> > label shadows label? Maybe oldLabel/newLabel? Then you can also reuse the `Label*` in line 52 and 59 (`label_`)
Good point - will get that in.
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30961/#review79454
-----------------------------------------------------------
On April 7, 2015, 5:57 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30961/
> -----------------------------------------------------------
>
> (Updated April 7, 2015, 5:57 p.m.)
>
>
> Review request for mesos, Ben Mahler and Kapil Arya.
>
>
> Bugs: MESOS-2351
> https://issues.apache.org/jira/browse/MESOS-2351
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp 9ae8b9455a86c7a5cbf4f1d1b1ce88f2811ce35d
> src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41
> src/hook/manager.cpp 7a4cb09bc221af502e867cfb7fff2900b599ff1f
> src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b
> src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
>
> Diff: https://reviews.apache.org/r/30961/diff/
>
>
> Testing
> -------
>
> make check (with modified VerifyMasterLaunchTaskHook test)
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 30961: Enabled label decorator to override.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On April 8, 2015, 5:31 p.m., Adam B wrote:
> > LGTM. Where's the JIRA/patch for updating the upgrades doc to explain the module API change?
Here :) https://issues.apache.org/jira/browse/MESOS-2622
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30961/#review79454
-----------------------------------------------------------
On April 7, 2015, 5:57 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30961/
> -----------------------------------------------------------
>
> (Updated April 7, 2015, 5:57 p.m.)
>
>
> Review request for mesos, Ben Mahler and Kapil Arya.
>
>
> Bugs: MESOS-2351
> https://issues.apache.org/jira/browse/MESOS-2351
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp 9ae8b9455a86c7a5cbf4f1d1b1ce88f2811ce35d
> src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41
> src/hook/manager.cpp 7a4cb09bc221af502e867cfb7fff2900b599ff1f
> src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b
> src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
>
> Diff: https://reviews.apache.org/r/30961/diff/
>
>
> Testing
> -------
>
> make check (with modified VerifyMasterLaunchTaskHook test)
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 30961: Enabled label decorator to override.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30961/#review79454
-----------------------------------------------------------
Ship it!
LGTM. Where's the JIRA/patch for updating the upgrades doc to explain the module API change?
src/examples/test_hook_module.cpp
<https://reviews.apache.org/r/30961/#comment128844>
Unused? Or should you check that the value being removed is what you expect?
src/examples/test_hook_module.cpp
<https://reviews.apache.org/r/30961/#comment128845>
label shadows label? Maybe oldLabel/newLabel? Then you can also reuse the `Label*` in line 52 and 59 (`label_`)
- Adam B
On April 7, 2015, 5:57 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30961/
> -----------------------------------------------------------
>
> (Updated April 7, 2015, 5:57 p.m.)
>
>
> Review request for mesos, Ben Mahler and Kapil Arya.
>
>
> Bugs: MESOS-2351
> https://issues.apache.org/jira/browse/MESOS-2351
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp 9ae8b9455a86c7a5cbf4f1d1b1ce88f2811ce35d
> src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41
> src/hook/manager.cpp 7a4cb09bc221af502e867cfb7fff2900b599ff1f
> src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b
> src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
>
> Diff: https://reviews.apache.org/r/30961/diff/
>
>
> Testing
> -------
>
> make check (with modified VerifyMasterLaunchTaskHook test)
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 30961: Enabled label decorator to override.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30961/
-----------------------------------------------------------
(Updated April 7, 2015, 5:57 p.m.)
Review request for mesos, Ben Mahler and Kapil Arya.
Changes
-------
Addressed Adam's and Kapil's comments.
Bugs: MESOS-2351
https://issues.apache.org/jira/browse/MESOS-2351
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
include/mesos/hook.hpp 9ae8b9455a86c7a5cbf4f1d1b1ce88f2811ce35d
src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41
src/hook/manager.cpp 7a4cb09bc221af502e867cfb7fff2900b599ff1f
src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b
src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
Diff: https://reviews.apache.org/r/30961/diff/
Testing
-------
make check (with modified VerifyMasterLaunchTaskHook test)
Thanks,
Niklas Nielsen
Re: Review Request 30961: Enabled label decorator to override.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On April 7, 2015, 1:41 a.m., Adam B wrote:
> > Minor suggestions.
Thanks for the review Adam!
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30961/#review79150
-----------------------------------------------------------
On April 7, 2015, 5:57 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30961/
> -----------------------------------------------------------
>
> (Updated April 7, 2015, 5:57 p.m.)
>
>
> Review request for mesos, Ben Mahler and Kapil Arya.
>
>
> Bugs: MESOS-2351
> https://issues.apache.org/jira/browse/MESOS-2351
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp 9ae8b9455a86c7a5cbf4f1d1b1ce88f2811ce35d
> src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41
> src/hook/manager.cpp 7a4cb09bc221af502e867cfb7fff2900b599ff1f
> src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b
> src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
>
> Diff: https://reviews.apache.org/r/30961/diff/
>
>
> Testing
> -------
>
> make check (with modified VerifyMasterLaunchTaskHook test)
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 30961: Enabled label decorator to override.
Posted by Adam B <ad...@mesosphere.io>.
> On April 7, 2015, 1:41 a.m., Adam B wrote:
> > src/hook/manager.cpp, line 104
> > <https://reviews.apache.org/r/30961/diff/6/?file=914058#file914058line104>
> >
> > Would it make sense to make taskInfo a pass-by-value param, forcing the copy at the call?
>
> Niklas Nielsen wrote:
> That unfortunately changes the module API and needs to be updated quite a few spaces (compared to this single copy).
SGTM.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30961/#review79150
-----------------------------------------------------------
On April 7, 2015, 5:57 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30961/
> -----------------------------------------------------------
>
> (Updated April 7, 2015, 5:57 p.m.)
>
>
> Review request for mesos, Ben Mahler and Kapil Arya.
>
>
> Bugs: MESOS-2351
> https://issues.apache.org/jira/browse/MESOS-2351
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp 9ae8b9455a86c7a5cbf4f1d1b1ce88f2811ce35d
> src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41
> src/hook/manager.cpp 7a4cb09bc221af502e867cfb7fff2900b599ff1f
> src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b
> src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
>
> Diff: https://reviews.apache.org/r/30961/diff/
>
>
> Testing
> -------
>
> make check (with modified VerifyMasterLaunchTaskHook test)
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 30961: Enabled label decorator to override.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On April 7, 2015, 1:41 a.m., Adam B wrote:
> > src/hook/manager.cpp, line 104
> > <https://reviews.apache.org/r/30961/diff/6/?file=914058#file914058line104>
> >
> > Would it make sense to make taskInfo a pass-by-value param, forcing the copy at the call?
That unfortunately changes the module API and needs to be updated quite a few spaces (compared to this single copy).
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30961/#review79150
-----------------------------------------------------------
On April 7, 2015, 5:57 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30961/
> -----------------------------------------------------------
>
> (Updated April 7, 2015, 5:57 p.m.)
>
>
> Review request for mesos, Ben Mahler and Kapil Arya.
>
>
> Bugs: MESOS-2351
> https://issues.apache.org/jira/browse/MESOS-2351
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp 9ae8b9455a86c7a5cbf4f1d1b1ce88f2811ce35d
> src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41
> src/hook/manager.cpp 7a4cb09bc221af502e867cfb7fff2900b599ff1f
> src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b
> src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
>
> Diff: https://reviews.apache.org/r/30961/diff/
>
>
> Testing
> -------
>
> make check (with modified VerifyMasterLaunchTaskHook test)
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 30961: Enabled label decorator to override.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30961/#review79150
-----------------------------------------------------------
Minor suggestions.
src/hook/manager.cpp
<https://reviews.apache.org/r/30961/#comment128285>
Would it make sense to make taskInfo a pass-by-value param, forcing the copy at the call?
src/master/master.cpp
<https://reviews.apache.org/r/30961/#comment128286>
Update comment
src/tests/hook_tests.cpp
<https://reviews.apache.org/r/30961/#comment128287>
s/remove/removed/
s/hook/the hook/
src/tests/hook_tests.cpp
<https://reviews.apache.org/r/30961/#comment128288>
s/Value/Key/?
src/tests/hook_tests.cpp
<https://reviews.apache.org/r/30961/#comment128289>
Should also verify
`EXPECT_EQ(labels_.labels().Get(0).value(), testLabelValue);`
- Adam B
On April 2, 2015, 2:39 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30961/
> -----------------------------------------------------------
>
> (Updated April 2, 2015, 2:39 p.m.)
>
>
> Review request for mesos, Ben Mahler and Kapil Arya.
>
>
> Bugs: MESOS-2351
> https://issues.apache.org/jira/browse/MESOS-2351
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp 9ae8b9455a86c7a5cbf4f1d1b1ce88f2811ce35d
> src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41
> src/hook/manager.cpp 7a4cb09bc221af502e867cfb7fff2900b599ff1f
> src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b
> src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
>
> Diff: https://reviews.apache.org/r/30961/diff/
>
>
> Testing
> -------
>
> make check (with modified VerifyMasterLaunchTaskHook test)
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 30961: Enabled label decorator to override.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30961/
-----------------------------------------------------------
(Updated April 2, 2015, 2:39 p.m.)
Review request for mesos, Ben Mahler and Kapil Arya.
Changes
-------
Addressed Adam's and Kapil's comments.
Bugs: MESOS-2351
https://issues.apache.org/jira/browse/MESOS-2351
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
include/mesos/hook.hpp 9ae8b9455a86c7a5cbf4f1d1b1ce88f2811ce35d
src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41
src/hook/manager.cpp 7a4cb09bc221af502e867cfb7fff2900b599ff1f
src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b
src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
Diff: https://reviews.apache.org/r/30961/diff/
Testing
-------
make check (with modified VerifyMasterLaunchTaskHook test)
Thanks,
Niklas Nielsen
Re: Review Request 30961: Enabled label decorator to override.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30961/#review76816
-----------------------------------------------------------
Ship it!
src/examples/test_hook_module.cpp
<https://reviews.apache.org/r/30961/#comment124474>
s/MergeFrom/CopyFrom
- Kapil Arya
On March 13, 2015, 7:04 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30961/
> -----------------------------------------------------------
>
> (Updated March 13, 2015, 7:04 p.m.)
>
>
> Review request for mesos, Ben Mahler and Kapil Arya.
>
>
> Bugs: MESOS-2351
> https://issues.apache.org/jira/browse/MESOS-2351
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp 9ae8b9455a86c7a5cbf4f1d1b1ce88f2811ce35d
> src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41
> src/hook/manager.cpp 7a4cb09bc221af502e867cfb7fff2900b599ff1f
> src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1
> src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
>
> Diff: https://reviews.apache.org/r/30961/diff/
>
>
> Testing
> -------
>
> make check (with modified VerifyMasterLaunchTaskHook test)
>
>
> Thanks,
>
> Niklas Nielsen
>
>