You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joseph Wu <jo...@mesosphere.io> on 2019/08/19 21:55:57 UTC
Review Request 71315: Refactored master draining test setup.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71315/
-----------------------------------------------------------
Review request for mesos, Benno Evers and Greg Mann.
Bugs: MESOS-9892
https://issues.apache.org/jira/browse/MESOS-9892
Repository: mesos
Description
-------
Tests of this feature will generally require a master, agent, framework,
and a single task to be launched at the beginning of the test.
This moves this common code into the test SetUp.
This also changes the `post(...)` helper to return the http::Response
object instead of parsing it. The response for DRAIN_AGENT calls
does not return an object, so the tests were not checking the
response before.
Diffs
-----
src/tests/master_draining_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/71315/diff/1/
Testing
-------
make check
Thanks,
Joseph Wu
Re: Review Request 71315: Refactored master draining test setup.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71315/#review217437
-----------------------------------------------------------
Ship it!
Ship It!
- Greg Mann
On Aug. 22, 2019, 6:31 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71315/
> -----------------------------------------------------------
>
> (Updated Aug. 22, 2019, 6:31 p.m.)
>
>
> Review request for mesos, Benno Evers and Greg Mann.
>
>
> Bugs: MESOS-9892
> https://issues.apache.org/jira/browse/MESOS-9892
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Tests of this feature will generally require a master, agent, framework,
> and a single task to be launched at the beginning of the test.
> This moves this common code into the test SetUp.
>
> This also changes the `post(...)` helper to return the http::Response
> object instead of parsing it. The response for DRAIN_AGENT calls
> does not return an object, so the tests were not checking the
> response before.
>
>
> Diffs
> -----
>
> src/tests/master_draining_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71315/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71315: Refactored master draining test setup.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71315/
-----------------------------------------------------------
(Updated Aug. 22, 2019, 11:31 a.m.)
Review request for mesos, Benno Evers and Greg Mann.
Changes
-------
Removed Credential argument from HTTP Post helper function.
Bugs: MESOS-9892
https://issues.apache.org/jira/browse/MESOS-9892
Repository: mesos
Description
-------
Tests of this feature will generally require a master, agent, framework,
and a single task to be launched at the beginning of the test.
This moves this common code into the test SetUp.
This also changes the `post(...)` helper to return the http::Response
object instead of parsing it. The response for DRAIN_AGENT calls
does not return an object, so the tests were not checking the
response before.
Diffs (updated)
-----
src/tests/master_draining_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/71315/diff/2/
Changes: https://reviews.apache.org/r/71315/diff/1-2/
Testing
-------
make check
Thanks,
Joseph Wu
Re: Review Request 71315: Refactored master draining test setup.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Aug. 21, 2019, 3:59 p.m., Greg Mann wrote:
> > src/tests/master_draining_tests.cpp
> > Lines 206 (patched)
> > <https://reviews.apache.org/r/71315/diff/1/?file=2161829#file2161829line253>
> >
> > Should we make this value larger? We advance the clock by the agent reregistration timeout in these tests, which could be large?
>
> Joseph Wu wrote:
> I don't think it makes too much of a difference how large or small this value is. Larger values mean that advancing the clock by this interval may trigger different intervals too. The tests will need to account for this either way.
>
> Note: Later in the chain, I add some clock advances by this value, to test offers after reactivating agents.
kk, sg
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71315/#review217355
-----------------------------------------------------------
On Aug. 19, 2019, 9:55 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71315/
> -----------------------------------------------------------
>
> (Updated Aug. 19, 2019, 9:55 p.m.)
>
>
> Review request for mesos, Benno Evers and Greg Mann.
>
>
> Bugs: MESOS-9892
> https://issues.apache.org/jira/browse/MESOS-9892
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Tests of this feature will generally require a master, agent, framework,
> and a single task to be launched at the beginning of the test.
> This moves this common code into the test SetUp.
>
> This also changes the `post(...)` helper to return the http::Response
> object instead of parsing it. The response for DRAIN_AGENT calls
> does not return an object, so the tests were not checking the
> response before.
>
>
> Diffs
> -----
>
> src/tests/master_draining_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71315/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71315: Refactored master draining test setup.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Aug. 21, 2019, 3:59 p.m., Greg Mann wrote:
> > src/tests/master_draining_tests.cpp
> > Lines 249 (patched)
> > <https://reviews.apache.org/r/71315/diff/1/?file=2161829#file2161829line296>
> >
> > Do we need to parametrize these by content type? I suspect we have enough coverage of the API's handling of different content types elsewhere.
>
> Joseph Wu wrote:
> I'd lean towards keeping this parameterization. The two variants mean we'll be serializing the agent draining protobufs to/from JSON. Most of these protobufs are not used anywhere else too.
kk, sgtm
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71315/#review217355
-----------------------------------------------------------
On Aug. 19, 2019, 9:55 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71315/
> -----------------------------------------------------------
>
> (Updated Aug. 19, 2019, 9:55 p.m.)
>
>
> Review request for mesos, Benno Evers and Greg Mann.
>
>
> Bugs: MESOS-9892
> https://issues.apache.org/jira/browse/MESOS-9892
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Tests of this feature will generally require a master, agent, framework,
> and a single task to be launched at the beginning of the test.
> This moves this common code into the test SetUp.
>
> This also changes the `post(...)` helper to return the http::Response
> object instead of parsing it. The response for DRAIN_AGENT calls
> does not return an object, so the tests were not checking the
> response before.
>
>
> Diffs
> -----
>
> src/tests/master_draining_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71315/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71315: Refactored master draining test setup.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Aug. 21, 2019, 8:59 a.m., Greg Mann wrote:
> > src/tests/master_draining_tests.cpp
> > Lines 206 (patched)
> > <https://reviews.apache.org/r/71315/diff/1/?file=2161829#file2161829line253>
> >
> > Should we make this value larger? We advance the clock by the agent reregistration timeout in these tests, which could be large?
I don't think it makes too much of a difference how large or small this value is. Larger values mean that advancing the clock by this interval may trigger different intervals too. The tests will need to account for this either way.
Note: Later in the chain, I add some clock advances by this value, to test offers after reactivating agents.
> On Aug. 21, 2019, 8:59 a.m., Greg Mann wrote:
> > src/tests/master_draining_tests.cpp
> > Lines 216 (patched)
> > <https://reviews.apache.org/r/71315/diff/1/?file=2161829#file2161829line263>
> >
> > Is this argument ever used?
Nope, I can remove it.
> On Aug. 21, 2019, 8:59 a.m., Greg Mann wrote:
> > src/tests/master_draining_tests.cpp
> > Lines 249 (patched)
> > <https://reviews.apache.org/r/71315/diff/1/?file=2161829#file2161829line296>
> >
> > Do we need to parametrize these by content type? I suspect we have enough coverage of the API's handling of different content types elsewhere.
I'd lean towards keeping this parameterization. The two variants mean we'll be serializing the agent draining protobufs to/from JSON. Most of these protobufs are not used anywhere else too.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71315/#review217355
-----------------------------------------------------------
On Aug. 19, 2019, 2:55 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71315/
> -----------------------------------------------------------
>
> (Updated Aug. 19, 2019, 2:55 p.m.)
>
>
> Review request for mesos, Benno Evers and Greg Mann.
>
>
> Bugs: MESOS-9892
> https://issues.apache.org/jira/browse/MESOS-9892
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Tests of this feature will generally require a master, agent, framework,
> and a single task to be launched at the beginning of the test.
> This moves this common code into the test SetUp.
>
> This also changes the `post(...)` helper to return the http::Response
> object instead of parsing it. The response for DRAIN_AGENT calls
> does not return an object, so the tests were not checking the
> response before.
>
>
> Diffs
> -----
>
> src/tests/master_draining_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71315/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71315: Refactored master draining test setup.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71315/#review217355
-----------------------------------------------------------
Nice refactor!! Love it
src/tests/master_draining_tests.cpp
Lines 206 (patched)
<https://reviews.apache.org/r/71315/#comment304643>
Should we make this value larger? We advance the clock by the agent reregistration timeout in these tests, which could be large?
src/tests/master_draining_tests.cpp
Lines 216 (patched)
<https://reviews.apache.org/r/71315/#comment304644>
Is this argument ever used?
src/tests/master_draining_tests.cpp
Lines 249 (patched)
<https://reviews.apache.org/r/71315/#comment304645>
Do we need to parametrize these by content type? I suspect we have enough coverage of the API's handling of different content types elsewhere.
- Greg Mann
On Aug. 19, 2019, 9:55 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71315/
> -----------------------------------------------------------
>
> (Updated Aug. 19, 2019, 9:55 p.m.)
>
>
> Review request for mesos, Benno Evers and Greg Mann.
>
>
> Bugs: MESOS-9892
> https://issues.apache.org/jira/browse/MESOS-9892
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Tests of this feature will generally require a master, agent, framework,
> and a single task to be launched at the beginning of the test.
> This moves this common code into the test SetUp.
>
> This also changes the `post(...)` helper to return the http::Response
> object instead of parsing it. The response for DRAIN_AGENT calls
> does not return an object, so the tests were not checking the
> response before.
>
>
> Diffs
> -----
>
> src/tests/master_draining_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71315/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>