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:18 UTC
Re: Review Request 31028: Added slave run task hook tests.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31028/
-----------------------------------------------------------
(Updated March 13, 2015, 4:04 p.m.)
Review request for mesos, Ben Mahler and Kapil Arya.
Changes
-------
Rebased
Repository: mesos
Description
-------
See summary
Diffs (updated)
-----
src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41
src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
Diff: https://reviews.apache.org/r/31028/diff/
Testing
-------
make check (with newly added VerifySlaveRunTaskHook test)
Thanks,
Niklas Nielsen
Re: Review Request 31028: Added slave run task hook tests.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31028/#review79152
-----------------------------------------------------------
src/examples/test_hook_module.cpp
<https://reviews.apache.org/r/31028/#comment128291>
The foreach `label` shadows the `label` from line 79. And `label_` from line 86 doesn't need to be a different variable from the other `Label*` on line 79. Maybe oldLabel/newLabel are appropriate?
src/tests/hook_tests.cpp
<https://reviews.apache.org/r/31028/#comment128292>
No reason to explicitly CreateMasterFlags if you're not reusing `masterFlags` elsewhere or trying to force a new tmpDir.
src/tests/hook_tests.cpp
<https://reviews.apache.org/r/31028/#comment128293>
Unused.
src/tests/hook_tests.cpp
<https://reviews.apache.org/r/31028/#comment128295>
CopyFrom
src/tests/hook_tests.cpp
<https://reviews.apache.org/r/31028/#comment128296>
s/remove/removed/
src/tests/hook_tests.cpp
<https://reviews.apache.org/r/31028/#comment128297>
One which will... and the other...? Unclear what the second label is for (testing add)
- 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/31028/
> -----------------------------------------------------------
>
> (Updated March 13, 2015, 4:04 p.m.)
>
>
> Review request for mesos, Ben Mahler and Kapil Arya.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41
> src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
>
> Diff: https://reviews.apache.org/r/31028/diff/
>
>
> Testing
> -------
>
> make check (with newly added VerifySlaveRunTaskHook test)
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 31028: Added slave run task hook tests.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31028/#review76826
-----------------------------------------------------------
Ship it!
src/examples/test_hook_module.cpp
<https://reviews.apache.org/r/31028/#comment124486>
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/31028/
> -----------------------------------------------------------
>
> (Updated March 13, 2015, 7:04 p.m.)
>
>
> Review request for mesos, Ben Mahler and Kapil Arya.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41
> src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
>
> Diff: https://reviews.apache.org/r/31028/diff/
>
>
> Testing
> -------
>
> make check (with newly added VerifySlaveRunTaskHook test)
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 31028: Added slave run task hook tests.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On April 11, 2015, 3:54 a.m., Adam B wrote:
> > src/tests/hook_tests.cpp, lines 132-140
> > <https://reviews.apache.org/r/31028/diff/4/?file=920383#file920383line132>
> >
> > Why this change? What's wrong with the TestContainerizer?
I was running into issues as the test containerizer doesn't support using the command executor. Let me double check real quick if this is a moot change compared to the test refactor later in this patch set.
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31028/#review79804
-----------------------------------------------------------
On April 11, 2015, 3:03 a.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31028/
> -----------------------------------------------------------
>
> (Updated April 11, 2015, 3:03 a.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
> -----
>
> src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41
> src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
>
> Diff: https://reviews.apache.org/r/31028/diff/
>
>
> Testing
> -------
>
> make check (with newly added VerifySlaveRunTaskHook test)
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 31028: Added slave run task hook tests.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On April 11, 2015, 3:54 a.m., Adam B wrote:
> > src/examples/test_hook_module.cpp, lines 80-85
> > <https://reviews.apache.org/r/31028/diff/4/?file=920382#file920382line80>
> >
> > Create variables like testLabelKey, etc. above so it's easier to track all these label k/v strings.
I would prefer if we could defer this to a subsequent review; we use foo, bar, baz, qux quite a few places for label testing. Is that OK with you? I can create a JIRA for it now if you'd like.
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31028/#review79804
-----------------------------------------------------------
On April 11, 2015, 3:03 a.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31028/
> -----------------------------------------------------------
>
> (Updated April 11, 2015, 3:03 a.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
> -----
>
> src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41
> src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
>
> Diff: https://reviews.apache.org/r/31028/diff/
>
>
> Testing
> -------
>
> make check (with newly added VerifySlaveRunTaskHook test)
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 31028: Added slave run task hook tests.
Posted by Adam B <ad...@mesosphere.io>.
> On April 11, 2015, 3:54 a.m., Adam B wrote:
> > src/examples/test_hook_module.cpp, lines 80-85
> > <https://reviews.apache.org/r/31028/diff/4/?file=920382#file920382line80>
> >
> > Create variables like testLabelKey, etc. above so it's easier to track all these label k/v strings.
>
> Niklas Nielsen wrote:
> I would prefer if we could defer this to a subsequent review; we use foo, bar, baz, qux quite a few places for label testing. Is that OK with you? I can create a JIRA for it now if you'd like.
Separate review/JIRA is fine by me. I just have an aversion to hardcoded strings showing up in multiple places.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31028/#review79804
-----------------------------------------------------------
On April 11, 2015, 3:03 a.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31028/
> -----------------------------------------------------------
>
> (Updated April 11, 2015, 3:03 a.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
> -----
>
> src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41
> src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
>
> Diff: https://reviews.apache.org/r/31028/diff/
>
>
> Testing
> -------
>
> make check (with newly added VerifySlaveRunTaskHook test)
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 31028: Added slave run task hook tests.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On April 11, 2015, 3:54 a.m., Adam B wrote:
> > src/examples/test_hook_module.cpp, lines 80-85
> > <https://reviews.apache.org/r/31028/diff/4/?file=920382#file920382line80>
> >
> > Create variables like testLabelKey, etc. above so it's easier to track all these label k/v strings.
>
> Niklas Nielsen wrote:
> I would prefer if we could defer this to a subsequent review; we use foo, bar, baz, qux quite a few places for label testing. Is that OK with you? I can create a JIRA for it now if you'd like.
>
> Adam B wrote:
> Separate review/JIRA is fine by me. I just have an aversion to hardcoded strings showing up in multiple places.
https://issues.apache.org/jira/browse/MESOS-2622
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31028/#review79804
-----------------------------------------------------------
On April 20, 2015, 1:17 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31028/
> -----------------------------------------------------------
>
> (Updated April 20, 2015, 1:17 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
> -----
>
> src/examples/test_hook_module.cpp 2f2da1c5ef85af06c7f366d38ce5b64f39d0076f
> src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
>
> Diff: https://reviews.apache.org/r/31028/diff/
>
>
> Testing
> -------
>
> make check (with newly added VerifySlaveRunTaskHook test)
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 31028: Added slave run task hook tests.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31028/#review79804
-----------------------------------------------------------
src/examples/test_hook_module.cpp
<https://reviews.apache.org/r/31028/#comment129387>
Create variables like testLabelKey, etc. above so it's easier to track all these label k/v strings.
src/tests/hook_tests.cpp
<https://reviews.apache.org/r/31028/#comment129388>
Why this change? What's wrong with the TestContainerizer?
- Adam B
On April 11, 2015, 3:03 a.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31028/
> -----------------------------------------------------------
>
> (Updated April 11, 2015, 3:03 a.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
> -----
>
> src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41
> src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
>
> Diff: https://reviews.apache.org/r/31028/diff/
>
>
> Testing
> -------
>
> make check (with newly added VerifySlaveRunTaskHook test)
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 31028: Added slave run task hook tests.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On April 20, 2015, 2:12 p.m., Adam B wrote:
> > src/tests/hook_tests.cpp, lines 407-424
> > <https://reviews.apache.org/r/31028/diff/5/?file=936928#file936928line407>
> >
> > Would be great to see a diagram of these labels coming and going at different points in the runTask lifecycle. It's a little confusing keeping track of what happens when. New documentation JIRA?
https://issues.apache.org/jira/browse/MESOS-2639
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31028/#review80822
-----------------------------------------------------------
On April 20, 2015, 1:17 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31028/
> -----------------------------------------------------------
>
> (Updated April 20, 2015, 1:17 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
> -----
>
> src/examples/test_hook_module.cpp 2f2da1c5ef85af06c7f366d38ce5b64f39d0076f
> src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
>
> Diff: https://reviews.apache.org/r/31028/diff/
>
>
> Testing
> -------
>
> make check (with newly added VerifySlaveRunTaskHook test)
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 31028: Added slave run task hook tests.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31028/#review80822
-----------------------------------------------------------
Ship it!
Looks great!
src/tests/hook_tests.cpp
<https://reviews.apache.org/r/31028/#comment130932>
Would be great to see a diagram of these labels coming and going at different points in the runTask lifecycle. It's a little confusing keeping track of what happens when. New documentation JIRA?
- Adam B
On April 20, 2015, 1:17 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31028/
> -----------------------------------------------------------
>
> (Updated April 20, 2015, 1:17 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
> -----
>
> src/examples/test_hook_module.cpp 2f2da1c5ef85af06c7f366d38ce5b64f39d0076f
> src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
>
> Diff: https://reviews.apache.org/r/31028/diff/
>
>
> Testing
> -------
>
> make check (with newly added VerifySlaveRunTaskHook test)
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 31028: Added slave run task hook tests.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31028/
-----------------------------------------------------------
(Updated April 20, 2015, 1:17 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)
-----
src/examples/test_hook_module.cpp 2f2da1c5ef85af06c7f366d38ce5b64f39d0076f
src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
Diff: https://reviews.apache.org/r/31028/diff/
Testing
-------
make check (with newly added VerifySlaveRunTaskHook test)
Thanks,
Niklas Nielsen
Re: Review Request 31028: Added slave run task hook tests.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31028/
-----------------------------------------------------------
(Updated April 11, 2015, 3:03 a.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
-----
src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41
src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
Diff: https://reviews.apache.org/r/31028/diff/
Testing
-------
make check (with newly added VerifySlaveRunTaskHook test)
Thanks,
Niklas Nielsen
Re: Review Request 31028: Added slave run task hook tests.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31028/
-----------------------------------------------------------
(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.
Repository: mesos
Description
-------
See summary
Diffs (updated)
-----
src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41
src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378
Diff: https://reviews.apache.org/r/31028/diff/
Testing
-------
make check (with newly added VerifySlaveRunTaskHook test)
Thanks,
Niklas Nielsen