You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Thomas Marshall <tw...@gmail.com> on 2013/07/08 23:51:30 UTC
Review Request 12341: Reservations 2 - Add roles to Resources::parse
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12341/
-----------------------------------------------------------
Review request for mesos and Benjamin Hindman.
Bugs: MESOS-505
https://issues.apache.org/jira/browse/MESOS-505
Repository: mesos
Description
-------
Adds parsing of roles to the Resources class. Also updates parse to return a Try, in the case that parsing fails, and adds tests for role parsing and for Resources operators in the presence of roles.
Diffs
-----
src/common/resources.hpp 42dfb6a
src/common/resources.cpp 8bc1057
src/master/drf_sorter.cpp 510acc3
src/slave/slave.cpp 9a2c671
src/tests/gc_tests.cpp 1745800
src/tests/master_tests.cpp 5ac4d5f
src/tests/resource_offers_tests.cpp beab2d8
src/tests/resources_tests.cpp 67a11b3
src/tests/slave_recovery_tests.cpp b0843d7
src/tests/sorter_tests.cpp 619e786
Diff: https://reviews.apache.org/r/12341/diff/
Testing
-------
make check
Thanks,
Thomas Marshall
Re: Review Request 12341: Reservations 2 - Add roles to Resources::parse
Posted by Thomas Marshall <tw...@gmail.com>.
> On July 15, 2013, 7:30 p.m., Ben Mahler wrote:
> > Can you provide some context for the use of '=' vs ':' in the resource strings?
> >
> > "cpus=45.55 (*); ports=[10000-20000, 30000-50000] (*); disks={sda1} (*)"
> >
> > vs.
> >
> > "cpus:45.55 (*); ports:[10000-20000, 30000-50000] (*); disks:{sda1} (*)"
We were merging the parse and stringify formats - parse used ':' and stringify used '='. The choice of ':' was arbitrary, and I'm certainly open to arguments about why one would be better than the other.
> On July 15, 2013, 7:30 p.m., Ben Mahler wrote:
> > src/tests/resources_tests.cpp, line 89
> > <https://reviews.apache.org/r/12341/diff/2/?file=321794#file321794line89>
> >
> > EXPECT?
Sorry about that.
> On July 15, 2013, 7:30 p.m., Ben Mahler wrote:
> > src/tests/resources_tests.cpp, lines 254-255
> > <https://reviews.apache.org/r/12341/diff/2/?file=321794#file321794line254>
> >
> > EXPECT_LE
I don't think that works - since we're expecting it to be false, we obviously can't use EXPECT_LE, and we also can't just do the reverse and use EXPECT_GT, since cpus1 > cpus2 should also be false. (admittedly, I'm not crazy about these semantics, since if a <= b is false, a > b should be true, but given the difficulty of comparing Resources, I'm not sure how else to make it work).
> On July 15, 2013, 7:30 p.m., Ben Mahler wrote:
> > src/tests/resources_tests.cpp, lines 262-265
> > <https://reviews.apache.org/r/12341/diff/2/?file=321794#file321794line262>
> >
> > Can you use EXPECT_LE?
Same as above, except for the EXPECT_TRUE.
- Thomas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12341/#review23169
-----------------------------------------------------------
On July 15, 2013, 6:36 p.m., Thomas Marshall wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12341/
> -----------------------------------------------------------
>
> (Updated July 15, 2013, 6:36 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-505
> https://issues.apache.org/jira/browse/MESOS-505
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds parsing of roles to the Resources class. Also updates parse to return a Try, in the case that parsing fails, and adds tests for role parsing and for Resources operators in the presence of roles.
>
>
> Diffs
> -----
>
> src/common/resources.hpp 42dfb6a
> src/common/resources.cpp 8bc1057
> src/master/drf_sorter.cpp 510acc3
> src/slave/slave.cpp 9a2c671
> src/tests/gc_tests.cpp 1745800
> src/tests/master_tests.cpp 5ac4d5f
> src/tests/resource_offers_tests.cpp beab2d8
> src/tests/resources_tests.cpp 67a11b3
> src/tests/slave_recovery_tests.cpp b0843d7
> src/tests/sorter_tests.cpp 619e786
>
> Diff: https://reviews.apache.org/r/12341/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Thomas Marshall
>
>
Re: Review Request 12341: Reservations 2 - Add roles to Resources::parse
Posted by Ben Mahler <be...@gmail.com>.
> On July 15, 2013, 7:30 p.m., Ben Mahler wrote:
> > Can you provide some context for the use of '=' vs ':' in the resource strings?
> >
> > "cpus=45.55 (*); ports=[10000-20000, 30000-50000] (*); disks={sda1} (*)"
> >
> > vs.
> >
> > "cpus:45.55 (*); ports:[10000-20000, 30000-50000] (*); disks:{sda1} (*)"
>
> Thomas Marshall wrote:
> We were merging the parse and stringify formats - parse used ':' and stringify used '='. The choice of ':' was arbitrary, and I'm certainly open to arguments about why one would be better than the other.
>
> Thomas Marshall wrote:
> It occurs to me - an argument in favor of keeping it ':' is that people who are specifying --resources to the slave won't have to change the format.
That's a good point, we should keep it as ':'. What about other formats for the role? We should carefully consider the format as it's going to be very difficult to change it once released! ;)
Current:
(1) cpus=4 (*); cpus=8 (jenkins); memory=10GB (*); memory=20GB (jenkins)
How about:
(2) cpus(*):4; cpus(jenkins):8; memory(*):10GB; memory(jenkins):20GB
I would suggest 2 (benh is in agreement on this), should be easier to parse as well (using strings::split) :)
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12341/#review23169
-----------------------------------------------------------
On July 15, 2013, 10:23 p.m., Thomas Marshall wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12341/
> -----------------------------------------------------------
>
> (Updated July 15, 2013, 10:23 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-505
> https://issues.apache.org/jira/browse/MESOS-505
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds parsing of roles to the Resources class. Also updates parse to return a Try, in the case that parsing fails, and adds tests for role parsing and for Resources operators in the presence of roles.
>
>
> Diffs
> -----
>
> src/common/resources.hpp 42dfb6a
> src/common/resources.cpp 8bc1057
> src/master/drf_sorter.cpp 510acc3
> src/slave/slave.cpp 9a2c671
> src/tests/gc_tests.cpp 1745800
> src/tests/master_tests.cpp 5ac4d5f
> src/tests/resource_offers_tests.cpp beab2d8
> src/tests/resources_tests.cpp 67a11b3
> src/tests/slave_recovery_tests.cpp b0843d7
> src/tests/sorter_tests.cpp 619e786
>
> Diff: https://reviews.apache.org/r/12341/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Thomas Marshall
>
>
Re: Review Request 12341: Reservations 2 - Add roles to Resources::parse
Posted by Thomas Marshall <tw...@gmail.com>.
> On July 15, 2013, 7:30 p.m., Ben Mahler wrote:
> > Can you provide some context for the use of '=' vs ':' in the resource strings?
> >
> > "cpus=45.55 (*); ports=[10000-20000, 30000-50000] (*); disks={sda1} (*)"
> >
> > vs.
> >
> > "cpus:45.55 (*); ports:[10000-20000, 30000-50000] (*); disks:{sda1} (*)"
>
> Thomas Marshall wrote:
> We were merging the parse and stringify formats - parse used ':' and stringify used '='. The choice of ':' was arbitrary, and I'm certainly open to arguments about why one would be better than the other.
It occurs to me - an argument in favor of keeping it ':' is that people who are specifying --resources to the slave won't have to change the format.
- Thomas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12341/#review23169
-----------------------------------------------------------
On July 15, 2013, 10:23 p.m., Thomas Marshall wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12341/
> -----------------------------------------------------------
>
> (Updated July 15, 2013, 10:23 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-505
> https://issues.apache.org/jira/browse/MESOS-505
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds parsing of roles to the Resources class. Also updates parse to return a Try, in the case that parsing fails, and adds tests for role parsing and for Resources operators in the presence of roles.
>
>
> Diffs
> -----
>
> src/common/resources.hpp 42dfb6a
> src/common/resources.cpp 8bc1057
> src/master/drf_sorter.cpp 510acc3
> src/slave/slave.cpp 9a2c671
> src/tests/gc_tests.cpp 1745800
> src/tests/master_tests.cpp 5ac4d5f
> src/tests/resource_offers_tests.cpp beab2d8
> src/tests/resources_tests.cpp 67a11b3
> src/tests/slave_recovery_tests.cpp b0843d7
> src/tests/sorter_tests.cpp 619e786
>
> Diff: https://reviews.apache.org/r/12341/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Thomas Marshall
>
>
Re: Review Request 12341: Reservations 2 - Add roles to Resources::parse
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12341/#review23169
-----------------------------------------------------------
Can you provide some context for the use of '=' vs ':' in the resource strings?
"cpus=45.55 (*); ports=[10000-20000, 30000-50000] (*); disks={sda1} (*)"
vs.
"cpus:45.55 (*); ports:[10000-20000, 30000-50000] (*); disks:{sda1} (*)"
src/common/resources.cpp
<https://reviews.apache.org/r/12341/#comment47024>
foreach?
src/tests/resources_tests.cpp
<https://reviews.apache.org/r/12341/#comment47029>
EXPECT?
src/tests/resources_tests.cpp
<https://reviews.apache.org/r/12341/#comment47030>
EXPECT?
src/tests/resources_tests.cpp
<https://reviews.apache.org/r/12341/#comment47025>
EXPECT_ERROR here and elsewhere, then you don't need to store the Try at all :)
src/tests/resources_tests.cpp
<https://reviews.apache.org/r/12341/#comment47026>
EXPECT_NE, using it will then show the expected vs. actual in the test failure output (so long as they both have output stream operators).
src/tests/resources_tests.cpp
<https://reviews.apache.org/r/12341/#comment47028>
EXPECT_LE
src/tests/resources_tests.cpp
<https://reviews.apache.org/r/12341/#comment47027>
Can you use EXPECT_LE?
- Ben Mahler
On July 15, 2013, 6:36 p.m., Thomas Marshall wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12341/
> -----------------------------------------------------------
>
> (Updated July 15, 2013, 6:36 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-505
> https://issues.apache.org/jira/browse/MESOS-505
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds parsing of roles to the Resources class. Also updates parse to return a Try, in the case that parsing fails, and adds tests for role parsing and for Resources operators in the presence of roles.
>
>
> Diffs
> -----
>
> src/common/resources.hpp 42dfb6a
> src/common/resources.cpp 8bc1057
> src/master/drf_sorter.cpp 510acc3
> src/slave/slave.cpp 9a2c671
> src/tests/gc_tests.cpp 1745800
> src/tests/master_tests.cpp 5ac4d5f
> src/tests/resource_offers_tests.cpp beab2d8
> src/tests/resources_tests.cpp 67a11b3
> src/tests/slave_recovery_tests.cpp b0843d7
> src/tests/sorter_tests.cpp 619e786
>
> Diff: https://reviews.apache.org/r/12341/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Thomas Marshall
>
>
Re: Review Request 12341: Reservations 2 - Add roles to Resources::parse
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12341/#review23401
-----------------------------------------------------------
Ship it!
Cool.
src/common/resources.cpp
<https://reviews.apache.org/r/12341/#comment47268>
s/mismatches/mismatched/
src/slave/slave.cpp
<https://reviews.apache.org/r/12341/#comment47267>
The other convention we have is to use the function name: s/resources_/parse/. This "reads" pretty clearly too ("CHECK_SOME checks that the parse did not error and 'parse.get' returns the result of parse).
src/tests/resources_tests.cpp
<https://reviews.apache.org/r/12341/#comment47266>
s/RemoveRoles/FlattenRoles/
- Benjamin Hindman
On July 18, 2013, 12:25 a.m., Thomas Marshall wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12341/
> -----------------------------------------------------------
>
> (Updated July 18, 2013, 12:25 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-505
> https://issues.apache.org/jira/browse/MESOS-505
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds parsing of roles to the Resources class. Also updates parse to return a Try, in the case that parsing fails, and adds tests for role parsing and for Resources operators in the presence of roles.
>
>
> Diffs
> -----
>
> src/common/resources.hpp 42dfb6a
> src/common/resources.cpp 8bc1057
> src/master/drf_sorter.cpp 510acc3
> src/slave/slave.cpp 9a2c671
> src/tests/gc_tests.cpp 1745800
> src/tests/master_tests.cpp 5ac4d5f
> src/tests/resource_offers_tests.cpp beab2d8
> src/tests/resources_tests.cpp 67a11b3
> src/tests/slave_recovery_tests.cpp b0843d7
> src/tests/sorter_tests.cpp 619e786
>
> Diff: https://reviews.apache.org/r/12341/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Thomas Marshall
>
>
Re: Review Request 12341: Reservations 2 - Add roles to Resources::parse
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12341/#review23585
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Hindman
On July 18, 2013, 9:34 p.m., Thomas Marshall wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12341/
> -----------------------------------------------------------
>
> (Updated July 18, 2013, 9:34 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-505
> https://issues.apache.org/jira/browse/MESOS-505
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds parsing of roles to the Resources class. Also updates parse to return a Try, in the case that parsing fails, and adds tests for role parsing and for Resources operators in the presence of roles.
>
>
> Diffs
> -----
>
> src/common/resources.hpp 42dfb6a
> src/common/resources.cpp 8bc1057
> src/master/drf_sorter.cpp 510acc3
> src/slave/slave.cpp 9a2c671
> src/tests/gc_tests.cpp 1745800
> src/tests/master_tests.cpp 5ac4d5f
> src/tests/resource_offers_tests.cpp beab2d8
> src/tests/resources_tests.cpp 67a11b3
> src/tests/slave_recovery_tests.cpp b0843d7
> src/tests/sorter_tests.cpp 619e786
>
> Diff: https://reviews.apache.org/r/12341/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Thomas Marshall
>
>
Re: Review Request 12341: Reservations 2 - Add roles to Resources::parse
Posted by Thomas Marshall <tw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12341/
-----------------------------------------------------------
(Updated July 18, 2013, 9:34 p.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
Ben H's review.
Bugs: MESOS-505
https://issues.apache.org/jira/browse/MESOS-505
Repository: mesos
Description
-------
Adds parsing of roles to the Resources class. Also updates parse to return a Try, in the case that parsing fails, and adds tests for role parsing and for Resources operators in the presence of roles.
Diffs (updated)
-----
src/common/resources.hpp 42dfb6a
src/common/resources.cpp 8bc1057
src/master/drf_sorter.cpp 510acc3
src/slave/slave.cpp 9a2c671
src/tests/gc_tests.cpp 1745800
src/tests/master_tests.cpp 5ac4d5f
src/tests/resource_offers_tests.cpp beab2d8
src/tests/resources_tests.cpp 67a11b3
src/tests/slave_recovery_tests.cpp b0843d7
src/tests/sorter_tests.cpp 619e786
Diff: https://reviews.apache.org/r/12341/diff/
Testing
-------
make check
Thanks,
Thomas Marshall
Re: Review Request 12341: Reservations 2 - Add roles to Resources::parse
Posted by Thomas Marshall <tw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12341/
-----------------------------------------------------------
(Updated July 18, 2013, 12:25 a.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
Moved the role in the resource input/output format to be next to the resource name:
"cpus(role1):1;..."
Bugs: MESOS-505
https://issues.apache.org/jira/browse/MESOS-505
Repository: mesos
Description
-------
Adds parsing of roles to the Resources class. Also updates parse to return a Try, in the case that parsing fails, and adds tests for role parsing and for Resources operators in the presence of roles.
Diffs (updated)
-----
src/common/resources.hpp 42dfb6a
src/common/resources.cpp 8bc1057
src/master/drf_sorter.cpp 510acc3
src/slave/slave.cpp 9a2c671
src/tests/gc_tests.cpp 1745800
src/tests/master_tests.cpp 5ac4d5f
src/tests/resource_offers_tests.cpp beab2d8
src/tests/resources_tests.cpp 67a11b3
src/tests/slave_recovery_tests.cpp b0843d7
src/tests/sorter_tests.cpp 619e786
Diff: https://reviews.apache.org/r/12341/diff/
Testing
-------
make check
Thanks,
Thomas Marshall
Re: Review Request 12341: Reservations 2 - Add roles to Resources::parse
Posted by Thomas Marshall <tw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12341/
-----------------------------------------------------------
(Updated July 15, 2013, 10:23 p.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
Ben M's review.
Bugs: MESOS-505
https://issues.apache.org/jira/browse/MESOS-505
Repository: mesos
Description
-------
Adds parsing of roles to the Resources class. Also updates parse to return a Try, in the case that parsing fails, and adds tests for role parsing and for Resources operators in the presence of roles.
Diffs (updated)
-----
src/common/resources.hpp 42dfb6a
src/common/resources.cpp 8bc1057
src/master/drf_sorter.cpp 510acc3
src/slave/slave.cpp 9a2c671
src/tests/gc_tests.cpp 1745800
src/tests/master_tests.cpp 5ac4d5f
src/tests/resource_offers_tests.cpp beab2d8
src/tests/resources_tests.cpp 67a11b3
src/tests/slave_recovery_tests.cpp b0843d7
src/tests/sorter_tests.cpp 619e786
Diff: https://reviews.apache.org/r/12341/diff/
Testing
-------
make check
Thanks,
Thomas Marshall
Re: Review Request 12341: Reservations 2 - Add roles to Resources::parse
Posted by Thomas Marshall <tw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12341/
-----------------------------------------------------------
(Updated July 15, 2013, 6:36 p.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
Ben H and I decided that it would be better to have the input parse format be the same as the output stringify format for Resources, so both now use the format:
"name:value (role); name:value (role);..."
Bugs: MESOS-505
https://issues.apache.org/jira/browse/MESOS-505
Repository: mesos
Description
-------
Adds parsing of roles to the Resources class. Also updates parse to return a Try, in the case that parsing fails, and adds tests for role parsing and for Resources operators in the presence of roles.
Diffs (updated)
-----
src/common/resources.hpp 42dfb6a
src/common/resources.cpp 8bc1057
src/master/drf_sorter.cpp 510acc3
src/slave/slave.cpp 9a2c671
src/tests/gc_tests.cpp 1745800
src/tests/master_tests.cpp 5ac4d5f
src/tests/resource_offers_tests.cpp beab2d8
src/tests/resources_tests.cpp 67a11b3
src/tests/slave_recovery_tests.cpp b0843d7
src/tests/sorter_tests.cpp 619e786
Diff: https://reviews.apache.org/r/12341/diff/
Testing
-------
make check
Thanks,
Thomas Marshall