You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jay Guo <gu...@gmail.com> on 2017/02/07 07:16:35 UTC
Review Request 56375: Added a test to ensure offers with different
roles cannot be combined.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56375/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
Bugs: MESOS-6637
https://issues.apache.org/jira/browse/MESOS-6637
Repository: mesos
Description
-------
Multi-role framework cannot combine offers allocated to different
roles of that framework in a single launchTask call.
Diffs
-----
src/tests/master_tests.cpp 3b4123b49ee32c902a5d2a01fcc7026da21fdd18
Diff: https://reviews.apache.org/r/56375/diff/
Testing
-------
Added a test
make check GTEST_FILTER="MasterTest.LaunchDifferentRoleLost"
Thanks,
Jay Guo
Re: Review Request 56375: Added a test to ensure offers with
different roles cannot be combined.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56375/#review164812
-----------------------------------------------------------
Ship it!
Ship It!
- Michael Park
On Feb. 6, 2017, 11:34 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56375/
> -----------------------------------------------------------
>
> (Updated Feb. 6, 2017, 11:34 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Multi-role framework cannot combine offers allocated to different
> roles of that framework in a single launchTask call.
>
>
> Diffs
> -----
>
> src/tests/master_tests.cpp 3b4123b49ee32c902a5d2a01fcc7026da21fdd18
>
> Diff: https://reviews.apache.org/r/56375/diff/
>
>
> Testing
> -------
>
> Added a test
> make check GTEST_FILTER="MasterTest.LaunchDifferentRoleLost"
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 56375: Added a test to ensure offers with
different roles cannot be combined.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Feb. 11, 2017, 12:16 a.m., Benjamin Mahler wrote:
> > src/tests/master_tests.cpp, line 2032
> > <https://reviews.apache.org/r/56375/diff/2/?file=1626156#file1626156line2032>
> >
> > This is a little hard to figure out, can you document their allocations to describe how we know that fair sharing will make these offers to different roles? Also, it seems to me we don't know which of the two roles will get the initial offer?
>
> Jay Guo wrote:
> We could test combined offer as long as we can assert that 2nd and 3rd offers are made to different roles, even though the role of 1st offer is not deterministic, no?
Yes that's what I imagine we want here, the part that seems unclear is how we're guaranteeing that 2nd and 3rd offers are made to different roles. I would just suggest that we document for posterity how we set up the test to ensure this holds.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56375/#review165041
-----------------------------------------------------------
On Feb. 13, 2017, 7:17 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56375/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2017, 7:17 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Multi-role framework cannot combine offers allocated to different
> roles of that framework in a single launchTask call.
>
>
> Diffs
> -----
>
> src/tests/master_tests.cpp 9a7a953adbbbed0b37082bb0166d03b1388c80ae
>
> Diff: https://reviews.apache.org/r/56375/diff/
>
>
> Testing
> -------
>
> Added a test
> make check GTEST_FILTER="MasterTest.LaunchDifferentRoleLost"
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 56375: Added a test to ensure offers with
different roles cannot be combined.
Posted by Jay Guo <gu...@gmail.com>.
> On Feb. 11, 2017, 8:16 a.m., Benjamin Mahler wrote:
> > src/tests/master_tests.cpp, line 2032
> > <https://reviews.apache.org/r/56375/diff/2/?file=1626156#file1626156line2032>
> >
> > This is a little hard to figure out, can you document their allocations to describe how we know that fair sharing will make these offers to different roles? Also, it seems to me we don't know which of the two roles will get the initial offer?
We could test combined offer as long as we can assert that 2nd and 3rd offers are made to different roles, even though the role of 1st offer is not deterministic, no?
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56375/#review165041
-----------------------------------------------------------
On Feb. 7, 2017, 3:34 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56375/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2017, 3:34 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Multi-role framework cannot combine offers allocated to different
> roles of that framework in a single launchTask call.
>
>
> Diffs
> -----
>
> src/tests/master_tests.cpp 3b4123b49ee32c902a5d2a01fcc7026da21fdd18
>
> Diff: https://reviews.apache.org/r/56375/diff/
>
>
> Testing
> -------
>
> Added a test
> make check GTEST_FILTER="MasterTest.LaunchDifferentRoleLost"
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 56375: Added a test to ensure offers with
different roles cannot be combined.
Posted by Jay Guo <gu...@gmail.com>.
> On Feb. 11, 2017, 8:16 a.m., Benjamin Mahler wrote:
> > src/tests/master_tests.cpp, line 2032
> > <https://reviews.apache.org/r/56375/diff/2/?file=1626156#file1626156line2032>
> >
> > This is a little hard to figure out, can you document their allocations to describe how we know that fair sharing will make these offers to different roles? Also, it seems to me we don't know which of the two roles will get the initial offer?
>
> Jay Guo wrote:
> We could test combined offer as long as we can assert that 2nd and 3rd offers are made to different roles, even though the role of 1st offer is not deterministic, no?
>
> Benjamin Mahler wrote:
> Yes that's what I imagine we want here, the part that seems unclear is how we're guaranteeing that 2nd and 3rd offers are made to different roles. I would just suggest that we document for posterity how we set up the test to ensure this holds.
OK, it's documented in the comments for that test, please take a look.
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56375/#review165041
-----------------------------------------------------------
On Feb. 13, 2017, 3:17 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56375/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2017, 3:17 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Multi-role framework cannot combine offers allocated to different
> roles of that framework in a single launchTask call.
>
>
> Diffs
> -----
>
> src/tests/master_tests.cpp 9a7a953adbbbed0b37082bb0166d03b1388c80ae
>
> Diff: https://reviews.apache.org/r/56375/diff/
>
>
> Testing
> -------
>
> Added a test
> make check GTEST_FILTER="MasterTest.LaunchDifferentRoleLost"
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 56375: Added a test to ensure offers with
different roles cannot be combined.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56375/#review165041
-----------------------------------------------------------
src/tests/master_tests.cpp (lines 1905 - 1909)
<https://reviews.apache.org/r/56375/#comment237032>
Do you want to add a TODO to test the other operations as well?
src/tests/master_tests.cpp (lines 1932 - 1936)
<https://reviews.apache.org/r/56375/#comment236879>
We should probably clear_role() here in order to not assume that DEFAULT_FRAMEWORK_INFO has not set the role field.
src/tests/master_tests.cpp (lines 1986 - 1987)
<https://reviews.apache.org/r/56375/#comment237028>
Is it possible to pause the clock for the whole test to ensure we're not depending on time?
src/tests/master_tests.cpp (line 1993)
<https://reviews.apache.org/r/56375/#comment237030>
You can use the -> operator (e.g. `offers2->size()` or `offers2->at(0)`), feel free to sweep this file in a later patch.
src/tests/master_tests.cpp (lines 1997 - 1998)
<https://reviews.apache.org/r/56375/#comment237031>
Does EXPECT_SOME_EQ not work here?
src/tests/master_tests.cpp (line 2032)
<https://reviews.apache.org/r/56375/#comment237035>
This is a little hard to figure out, can you document their allocations to describe how we know that fair sharing will make these offers to different roles? Also, it seems to me we don't know which of the two roles will get the initial offer?
- Benjamin Mahler
On Feb. 7, 2017, 7:34 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56375/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2017, 7:34 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Multi-role framework cannot combine offers allocated to different
> roles of that framework in a single launchTask call.
>
>
> Diffs
> -----
>
> src/tests/master_tests.cpp 3b4123b49ee32c902a5d2a01fcc7026da21fdd18
>
> Diff: https://reviews.apache.org/r/56375/diff/
>
>
> Testing
> -------
>
> Added a test
> make check GTEST_FILTER="MasterTest.LaunchDifferentRoleLost"
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 56375: Added a test to ensure offers with
different roles cannot be combined.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56375/#review166271
-----------------------------------------------------------
Ship it!
src/tests/master_tests.cpp (lines 1885 - 1902)
<https://reviews.apache.org/r/56375/#comment238186>
Made some slight adjustments here:
```
// This test ensures that a multi-role framework cannot launch tasks with
// offers allocated to different roles of that framework in a single
// launchTasks call. We follow similar pattern in LaunchCombinedOfferTest.
//
// We launch a cluster with one master and one slave, and a framework
// with two roles. Firstly, total resources will be offered to one of
// the roles (we don't assume that it is deterministic as to which of
// the two roles are chosen first). We launch a task using half of the
// total resources. The other half will be returned to master and offered
// to the other role, since it has a lower share (0). Then we kill the
// task, half of resources will be offered to first role again, since
// the first has a lower share (0). At this point, two offers with
// different roles are outstanding and we can combine them in one
// `launchTasks` call. A non-partition-aware framework should
// receive TASK_LOST.
```
- Benjamin Mahler
On Feb. 20, 2017, 7:35 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56375/
> -----------------------------------------------------------
>
> (Updated Feb. 20, 2017, 7:35 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Multi-role framework cannot combine offers allocated to different
> roles of that framework in a single launchTask call.
>
>
> Diffs
> -----
>
> src/tests/master_tests.cpp 64443dc6e2ca4ab8f37269a0dced49908526649b
>
> Diff: https://reviews.apache.org/r/56375/diff/
>
>
> Testing
> -------
>
> Added a test
> make check GTEST_FILTER="MasterTest.LaunchDifferentRoleLost"
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 56375: Added a test to ensure offers with
different roles cannot be combined.
Posted by Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56375/
-----------------------------------------------------------
(Updated Feb. 20, 2017, 3:35 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
Changes
-------
rebase.
Bugs: MESOS-6637
https://issues.apache.org/jira/browse/MESOS-6637
Repository: mesos
Description
-------
Multi-role framework cannot combine offers allocated to different
roles of that framework in a single launchTask call.
Diffs (updated)
-----
src/tests/master_tests.cpp 64443dc6e2ca4ab8f37269a0dced49908526649b
Diff: https://reviews.apache.org/r/56375/diff/
Testing
-------
Added a test
make check GTEST_FILTER="MasterTest.LaunchDifferentRoleLost"
Thanks,
Jay Guo
Re: Review Request 56375: Added a test to ensure offers with
different roles cannot be combined.
Posted by Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56375/
-----------------------------------------------------------
(Updated Feb. 13, 2017, 3:17 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
Changes
-------
address BenM's comments and rebase.
Bugs: MESOS-6637
https://issues.apache.org/jira/browse/MESOS-6637
Repository: mesos
Description
-------
Multi-role framework cannot combine offers allocated to different
roles of that framework in a single launchTask call.
Diffs (updated)
-----
src/tests/master_tests.cpp 9a7a953adbbbed0b37082bb0166d03b1388c80ae
Diff: https://reviews.apache.org/r/56375/diff/
Testing
-------
Added a test
make check GTEST_FILTER="MasterTest.LaunchDifferentRoleLost"
Thanks,
Jay Guo
Re: Review Request 56375: Added a test to ensure offers with
different roles cannot be combined.
Posted by Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56375/#review165294
-----------------------------------------------------------
src/tests/master_tests.cpp (line 2032)
<https://reviews.apache.org/r/56375/#comment237151>
We could test combined offer as long as we can assert that 2nd and 3rd offers are made to different roles, even though the role of 1st offer is not deterministic, no?
- Jay Guo
On Feb. 7, 2017, 3:34 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56375/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2017, 3:34 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Multi-role framework cannot combine offers allocated to different
> roles of that framework in a single launchTask call.
>
>
> Diffs
> -----
>
> src/tests/master_tests.cpp 3b4123b49ee32c902a5d2a01fcc7026da21fdd18
>
> Diff: https://reviews.apache.org/r/56375/diff/
>
>
> Testing
> -------
>
> Added a test
> make check GTEST_FILTER="MasterTest.LaunchDifferentRoleLost"
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 56375: Added a test to ensure offers with
different roles cannot be combined.
Posted by Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56375/
-----------------------------------------------------------
(Updated Feb. 7, 2017, 3:34 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
Bugs: MESOS-6637
https://issues.apache.org/jira/browse/MESOS-6637
Repository: mesos
Description
-------
Multi-role framework cannot combine offers allocated to different
roles of that framework in a single launchTask call.
Diffs (updated)
-----
src/tests/master_tests.cpp 3b4123b49ee32c902a5d2a01fcc7026da21fdd18
Diff: https://reviews.apache.org/r/56375/diff/
Testing
-------
Added a test
make check GTEST_FILTER="MasterTest.LaunchDifferentRoleLost"
Thanks,
Jay Guo