You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2015/06/09 21:51:57 UTC
Review Request 35264: Added a slave integration test in MonitorTest.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35264/
-----------------------------------------------------------
Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
Bugs: MESOS-2818
https://issues.apache.org/jira/browse/MESOS-2818
Repository: mesos
Description
-------
Added a slave integration test in MonitorTest.
This test verifies the wiring between resource monitor and the slave.
Diffs
-----
src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
Diff: https://reviews.apache.org/r/35264/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 35264: Added a slave integration test in
MonitorTest.
Posted by Jie Yu <yu...@gmail.com>.
> On June 11, 2015, 6:40 p.m., Niklas Nielsen wrote:
> > src/tests/monitor_tests.cpp, line 22
> > <https://reviews.apache.org/r/35264/diff/3/?file=982299#file982299line22>
> >
> > C-style include before C++ includes :)
This is a C++ header:)
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35264/#review87599
-----------------------------------------------------------
On June 10, 2015, 11:13 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35264/
> -----------------------------------------------------------
>
> (Updated June 10, 2015, 11:13 p.m.)
>
>
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2818
> https://issues.apache.org/jira/browse/MESOS-2818
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a slave integration test in MonitorTest.
>
> This test verifies the wiring between resource monitor and the slave.
>
>
> Diffs
> -----
>
> src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
>
> Diff: https://reviews.apache.org/r/35264/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 35264: Added a slave integration test in
MonitorTest.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35264/#review87599
-----------------------------------------------------------
Ship it!
src/tests/monitor_tests.cpp
<https://reviews.apache.org/r/35264/#comment139998>
C-style include before C++ includes :)
src/tests/monitor_tests.cpp
<https://reviews.apache.org/r/35264/#comment139999>
'Executor' or 'Task' ?
- Niklas Nielsen
On June 10, 2015, 4:13 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35264/
> -----------------------------------------------------------
>
> (Updated June 10, 2015, 4:13 p.m.)
>
>
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2818
> https://issues.apache.org/jira/browse/MESOS-2818
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a slave integration test in MonitorTest.
>
> This test verifies the wiring between resource monitor and the slave.
>
>
> Diffs
> -----
>
> src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
>
> Diff: https://reviews.apache.org/r/35264/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 35264: Added a slave integration test in
MonitorTest.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35264/
-----------------------------------------------------------
(Updated June 10, 2015, 11:13 p.m.)
Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
Changes
-------
Review comments.
Bugs: MESOS-2818
https://issues.apache.org/jira/browse/MESOS-2818
Repository: mesos
Description
-------
Added a slave integration test in MonitorTest.
This test verifies the wiring between resource monitor and the slave.
Diffs (updated)
-----
src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
Diff: https://reviews.apache.org/r/35264/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 35264: Added a slave integration test in
MonitorTest.
Posted by Jie Yu <yu...@gmail.com>.
> On June 10, 2015, 9:58 p.m., Ben Mahler wrote:
> > src/tests/monitor_tests.cpp, line 243
> > <https://reviews.apache.org/r/35264/diff/2/?file=982150#file982150line243>
> >
> > This case is still ok for taking a const &, since it's not a temporary.
Fixed.
> On June 10, 2015, 9:58 p.m., Ben Mahler wrote:
> > src/tests/monitor_tests.cpp, line 58
> > <https://reviews.apache.org/r/35264/diff/2/?file=982150#file982150line58>
> >
> > Why not keep MonitorTest to be clear that these are unit tests, and introduce a MonitorIntegrationTest that inherits from MesosTest?
> >
> > I could imagine multiple test cases within integration, so seems better on the left hand side:
> >
> > MonitorIntegrationTest.RunningExecutor
> > MonitorIntegrationTest.TerminatedExecutors
> >
> > Just as an example.
Good idea! Done.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35264/#review87468
-----------------------------------------------------------
On June 10, 2015, 6:30 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35264/
> -----------------------------------------------------------
>
> (Updated June 10, 2015, 6:30 p.m.)
>
>
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2818
> https://issues.apache.org/jira/browse/MESOS-2818
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a slave integration test in MonitorTest.
>
> This test verifies the wiring between resource monitor and the slave.
>
>
> Diffs
> -----
>
> src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
>
> Diff: https://reviews.apache.org/r/35264/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 35264: Added a slave integration test in
MonitorTest.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35264/#review87468
-----------------------------------------------------------
Ship it!
src/tests/monitor_tests.cpp
<https://reviews.apache.org/r/35264/#comment139788>
Why not keep MonitorTest to be clear that these are unit tests, and introduce a MonitorIntegrationTest that inherits from MesosTest?
I could imagine multiple test cases within integration, so seems better on the left hand side:
MonitorIntegrationTest.RunningExecutor
MonitorIntegrationTest.TerminatedExecutors
Just as an example.
src/tests/monitor_tests.cpp
<https://reviews.apache.org/r/35264/#comment139789>
This case is still ok for taking a const &, since it's not a temporary.
src/tests/monitor_tests.cpp
<https://reviews.apache.org/r/35264/#comment139791>
Took me awhile to figure out that this was calling process::address(). :)
At least we should be adding the process:: qualifier here, no?
- Ben Mahler
On June 10, 2015, 6:30 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35264/
> -----------------------------------------------------------
>
> (Updated June 10, 2015, 6:30 p.m.)
>
>
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2818
> https://issues.apache.org/jira/browse/MESOS-2818
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a slave integration test in MonitorTest.
>
> This test verifies the wiring between resource monitor and the slave.
>
>
> Diffs
> -----
>
> src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
>
> Diff: https://reviews.apache.org/r/35264/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 35264: Added a slave integration test in
MonitorTest.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35264/
-----------------------------------------------------------
(Updated June 10, 2015, 6:30 p.m.)
Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
Changes
-------
Addressed review comments.
Bugs: MESOS-2818
https://issues.apache.org/jira/browse/MESOS-2818
Repository: mesos
Description
-------
Added a slave integration test in MonitorTest.
This test verifies the wiring between resource monitor and the slave.
Diffs (updated)
-----
src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
Diff: https://reviews.apache.org/r/35264/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 35264: Added a slave integration test in
MonitorTest.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35264/#review87332
-----------------------------------------------------------
Patch looks great!
Reviews applied: [35259, 35260, 35264]
All tests passed.
- Mesos ReviewBot
On June 9, 2015, 7:51 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35264/
> -----------------------------------------------------------
>
> (Updated June 9, 2015, 7:51 p.m.)
>
>
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2818
> https://issues.apache.org/jira/browse/MESOS-2818
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a slave integration test in MonitorTest.
>
> This test verifies the wiring between resource monitor and the slave.
>
>
> Diffs
> -----
>
> src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
>
> Diff: https://reviews.apache.org/r/35264/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 35264: Added a slave integration test in
MonitorTest.
Posted by Jie Yu <yu...@gmail.com>.
> On June 9, 2015, 10:12 p.m., Niklas Nielsen wrote:
> > src/tests/monitor_tests.cpp, line 221
> > <https://reviews.apache.org/r/35264/diff/1/?file=981687#file981687line221>
> >
> > 77 lines of straight line code; Can we easen up a bit and add structure with some comments explaining what's going on and why?
Added comments.
> On June 9, 2015, 10:12 p.m., Niklas Nielsen wrote:
> > src/tests/monitor_tests.cpp, lines 282-292
> > <https://reviews.apache.org/r/35264/diff/1/?file=981687#file981687line282>
> >
> > Could you use .contains() instead? We should have landed and deal with matching fractions of JSON objects.
Good idea! Much cleaner!
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35264/#review87308
-----------------------------------------------------------
On June 9, 2015, 7:51 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35264/
> -----------------------------------------------------------
>
> (Updated June 9, 2015, 7:51 p.m.)
>
>
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2818
> https://issues.apache.org/jira/browse/MESOS-2818
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a slave integration test in MonitorTest.
>
> This test verifies the wiring between resource monitor and the slave.
>
>
> Diffs
> -----
>
> src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
>
> Diff: https://reviews.apache.org/r/35264/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 35264: Added a slave integration test in
MonitorTest.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35264/#review87308
-----------------------------------------------------------
LGTM modulo commenting on the sequence.
Also, could we model a JSON object and use contains() to check the result?
src/tests/monitor_tests.cpp
<https://reviews.apache.org/r/35264/#comment139649>
77 lines of straight line code; Can we easen up a bit and add structure with some comments explaining what's going on and why?
src/tests/monitor_tests.cpp
<https://reviews.apache.org/r/35264/#comment139650>
Do you need a copy or do you want to const this?
src/tests/monitor_tests.cpp
<https://reviews.apache.org/r/35264/#comment139651>
Could you use .contains() instead? We should have landed and deal with matching fractions of JSON objects.
- Niklas Nielsen
On June 9, 2015, 12:51 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35264/
> -----------------------------------------------------------
>
> (Updated June 9, 2015, 12:51 p.m.)
>
>
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2818
> https://issues.apache.org/jira/browse/MESOS-2818
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a slave integration test in MonitorTest.
>
> This test verifies the wiring between resource monitor and the slave.
>
>
> Diffs
> -----
>
> src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
>
> Diff: https://reviews.apache.org/r/35264/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>