You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Mahler <bm...@apache.org> on 2017/01/24 02:31:37 UTC
Review Request 55868: Cleanups to the allocator tests.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55868/
-----------------------------------------------------------
Review request for mesos and Michael Park.
Repository: mesos
Description
-------
This was necessary to greatly simplify the changes needed to the
allocator tests as we introduce support for multi-role frameworks.
The main improvement here is to establish and use equality on the
`Allocation` struct, which makes the tests more readable and avoids
the manual probing of the allocation structure across all the tests.
Diffs
-----
src/tests/hierarchical_allocator_tests.cpp 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5
Diff: https://reviews.apache.org/r/55868/diff/
Testing
-------
make check
Thanks,
Benjamin Mahler
Re: Review Request 55868: Cleanups to the allocator tests.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Jan. 26, 2017, 5:14 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 1950
> > <https://reviews.apache.org/r/55868/diff/1/?file=1613285#file1613285line1950>
> >
> > Any reason you want to update here?
This was a case where it was safe to use EXPECT instead of ASSERT.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55868/#review163096
-----------------------------------------------------------
On Jan. 24, 2017, 2:31 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55868/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2017, 2:31 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This was necessary to greatly simplify the changes needed to the
> allocator tests as we introduce support for multi-role frameworks.
>
> The main improvement here is to establish and use equality on the
> `Allocation` struct, which makes the tests more readable and avoids
> the manual probing of the allocation structure across all the tests.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5
>
> Diff: https://reviews.apache.org/r/55868/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55868: Cleanups to the allocator tests.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55868/#review163096
-----------------------------------------------------------
src/tests/hierarchical_allocator_tests.cpp (line 1870)
<https://reviews.apache.org/r/55868/#comment234549>
Any reason you want to update here?
- Guangya Liu
On \u4e00\u6708 24, 2017, 2:31 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55868/
> -----------------------------------------------------------
>
> (Updated \u4e00\u6708 24, 2017, 2:31 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This was necessary to greatly simplify the changes needed to the
> allocator tests as we introduce support for multi-role frameworks.
>
> The main improvement here is to establish and use equality on the
> `Allocation` struct, which makes the tests more readable and avoids
> the manual probing of the allocation structure across all the tests.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5
>
> Diff: https://reviews.apache.org/r/55868/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55868: Cleanups to the allocator tests.
Posted by Michael Park <mp...@apache.org>.
> On Jan. 30, 2017, 8:11 a.m., Benjamin Bannier wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 85
> > <https://reviews.apache.org/r/55868/diff/1/?file=1613285#file1613285line85>
> >
> > This should just be a plain constructor, right?
+1
> On Jan. 30, 2017, 8:11 a.m., Benjamin Bannier wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 289-293
> > <https://reviews.apache.org/r/55868/diff/1/?file=1613285#file1613285line289>
> >
> > If we made `Allocation::create` just a constructor, it should be possible to create `Allocation`s in test assertions, e.g.,
> >
> > AWAIT_EXPECT_EQ(Allocation(framework1.id(), {slave1.id(), slave1.resources()}),
> > allocations.get());
> >
> > Here and below.
> >
> > This should also help remove instances where `expected` is reused to hold completely different values.
> >
> > What do you think?
Well, to be fair, this is possible with `Allocation::create` too.
> On Jan. 30, 2017, 8:11 a.m., Benjamin Bannier wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 3530-3531
> > <https://reviews.apache.org/r/55868/diff/1/?file=1613285#file1613285line3530>
> >
> > This does look like Google style while we seem to prefer passing a non-`const` ref in such instances.
> >
> > Otherwise lets at least `CHECK_NOTNULL`.
The general rule is that pointer to non-`const` is preferred over non-`const` references for function parameters.
The reason (as far as I see it) is that we can't tell at the callsite whether an argument is to be modified or not.
We can talk about this outside of this review if we want.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55868/#review163524
-----------------------------------------------------------
On Jan. 23, 2017, 6:31 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55868/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2017, 6:31 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This was necessary to greatly simplify the changes needed to the
> allocator tests as we introduce support for multi-role frameworks.
>
> The main improvement here is to establish and use equality on the
> `Allocation` struct, which makes the tests more readable and avoids
> the manual probing of the allocation structure across all the tests.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5
>
> Diff: https://reviews.apache.org/r/55868/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55868: Cleanups to the allocator tests.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55868/#review163524
-----------------------------------------------------------
src/tests/hierarchical_allocator_tests.cpp (line 85)
<https://reviews.apache.org/r/55868/#comment234984>
This should just be a plain constructor, right?
src/tests/hierarchical_allocator_tests.cpp (lines 289 - 293)
<https://reviews.apache.org/r/55868/#comment234985>
If we made `Allocation::create` just a constructor, it should be possible to create `Allocation`s in test assertions, e.g.,
AWAIT_EXPECT_EQ(Allocation(framework1.id(), {slave1.id(), slave1.resources()}),
allocations.get());
Here and below.
This should also help remove instances where `expected` is reused to hold completely different values.
What do you think?
src/tests/hierarchical_allocator_tests.cpp (lines 3367 - 3368)
<https://reviews.apache.org/r/55868/#comment234986>
This does look like Google style while we seem to prefer passing a non-`const` ref in such instances.
Otherwise lets at least `CHECK_NOTNULL`.
- Benjamin Bannier
On Jan. 24, 2017, 3:31 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55868/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2017, 3:31 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This was necessary to greatly simplify the changes needed to the
> allocator tests as we introduce support for multi-role frameworks.
>
> The main improvement here is to establish and use equality on the
> `Allocation` struct, which makes the tests more readable and avoids
> the manual probing of the allocation structure across all the tests.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5
>
> Diff: https://reviews.apache.org/r/55868/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55868: Cleanups to the allocator tests.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Jan. 26, 2017, 5:05 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 948
> > <https://reviews.apache.org/r/55868/diff/1/?file=1613285#file1613285line948>
> >
> > I saw that you removed all of this `ASSERT_EQ` in the following tests, any reason you want do this?
Yes, they are no longer needed now that we are doing allocation equality.
> On Jan. 26, 2017, 5:05 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 1091
> > <https://reviews.apache.org/r/55868/diff/1/?file=1613285#file1613285line1091>
> >
> > How about move this right before `unreserved` under #1089?
Hm.. it seems clearer to have reserved and unreserved defined together?
> On Jan. 26, 2017, 5:05 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 4488
> > <https://reviews.apache.org/r/55868/diff/1/?file=1613285#file1613285line4488>
> >
> > s/allocations/offers
Nice catch! Thanks!
> On Jan. 26, 2017, 5:05 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 1505-1507
> > <https://reviews.apache.org/r/55868/diff/1/?file=1613285#file1613285line1505>
> >
> > How about move this right before the `update` used right before #1447 as before?
It's moved up to ensure that it's valid before we send it to the allocator.
> On Jan. 26, 2017, 5:05 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 1623
> > <https://reviews.apache.org/r/55868/diff/1/?file=1613285#file1613285line1623>
> >
> > Why not put this right before #1507 and use `allocation` for #1511 and #1522 as
> >
> > ```
> > AWAIT_EXPECT_EQ(expected, allocation);
> > ```
> >
> > Ditto for the following places.
The code doesn't need the allocation, so I could have written this as:
```
EXPECT_TRUE(allocations.get().isPending());
```
But the following seemed a bit clearer to read:
```
Future<Allocation> allocation = allocations.get();
EXPECT_TRUE(allocation.isPending());
```
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55868/#review163087
-----------------------------------------------------------
On Jan. 24, 2017, 2:31 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55868/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2017, 2:31 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This was necessary to greatly simplify the changes needed to the
> allocator tests as we introduce support for multi-role frameworks.
>
> The main improvement here is to establish and use equality on the
> `Allocation` struct, which makes the tests more readable and avoids
> the manual probing of the allocation structure across all the tests.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5
>
> Diff: https://reviews.apache.org/r/55868/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55868: Cleanups to the allocator tests.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55868/#review163087
-----------------------------------------------------------
src/tests/hierarchical_allocator_tests.cpp
<https://reviews.apache.org/r/55868/#comment234528>
I saw that you removed all of this `ASSERT_EQ` in the following tests, any reason you want do this?
src/tests/hierarchical_allocator_tests.cpp (line 1072)
<https://reviews.apache.org/r/55868/#comment234530>
How about move this right before `unreserved` under #1089?
src/tests/hierarchical_allocator_tests.cpp (lines 1315 - 1316)
<https://reviews.apache.org/r/55868/#comment234540>
How about udpate the comments here as well to hightlight `update`?
```
// Now recover the amount of `update` resources which is equal to
// `slave.resources()` and expect the next allocation equal to
// `slave.resources()`.
```
src/tests/hierarchical_allocator_tests.cpp (line 1316)
<https://reviews.apache.org/r/55868/#comment234539>
s/to equal/equal to
src/tests/hierarchical_allocator_tests.cpp (lines 1436 - 1438)
<https://reviews.apache.org/r/55868/#comment234541>
How about move this right before the `update` used right before #1447 as before?
src/tests/hierarchical_allocator_tests.cpp (line 1543)
<https://reviews.apache.org/r/55868/#comment234543>
Why not put this right before #1507 and use `allocation` for #1511 and #1522 as
```
AWAIT_EXPECT_EQ(expected, allocation);
```
Ditto for the following places.
src/tests/hierarchical_allocator_tests.cpp (line 4262)
<https://reviews.apache.org/r/55868/#comment234547>
s/allocations/offers
- Guangya Liu
On \u4e00\u6708 24, 2017, 2:31 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55868/
> -----------------------------------------------------------
>
> (Updated \u4e00\u6708 24, 2017, 2:31 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This was necessary to greatly simplify the changes needed to the
> allocator tests as we introduce support for multi-role frameworks.
>
> The main improvement here is to establish and use equality on the
> `Allocation` struct, which makes the tests more readable and avoids
> the manual probing of the allocation structure across all the tests.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5
>
> Diff: https://reviews.apache.org/r/55868/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55868: Cleanups to the allocator tests.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Feb. 1, 2017, 1:27 a.m., Jiang Yan Xu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 4357
> > <https://reviews.apache.org/r/55868/diff/1/?file=1613285#file1613285line4357>
> >
> > re Allocation vs. OfferedResources:
> >
> > Guangya and I chatted a bunch on /r/50868/.
> >
> > Seems like we can just use a single `struct Allocation` defined at the top of the file (which you are improving here) throughout without getting into a semantic differences between offers vs. allocation (at least right now). (In this file the choice between a OfferedResources vs. Allocation seems arbitrary)
> >
> > Thoughts?
Yeah I noticed this as well but didn't want to tackle that cleanup here.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55868/#review163768
-----------------------------------------------------------
On Jan. 24, 2017, 2:31 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55868/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2017, 2:31 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This was necessary to greatly simplify the changes needed to the
> allocator tests as we introduce support for multi-role frameworks.
>
> The main improvement here is to establish and use equality on the
> `Allocation` struct, which makes the tests more readable and avoids
> the manual probing of the allocation structure across all the tests.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5
>
> Diff: https://reviews.apache.org/r/55868/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55868: Cleanups to the allocator tests.
Posted by Jiang Yan Xu <xu...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55868/#review163768
-----------------------------------------------------------
src/tests/hierarchical_allocator_tests.cpp (line 4136)
<https://reviews.apache.org/r/55868/#comment235271>
re Allocation vs. OfferedResources:
Guangya and I chatted a bunch on /r/50868/.
Seems like we can just use a single `struct Allocation` defined at the top of the file (which you are improving here) throughout without getting into a semantic differences between offers vs. allocation (at least right now). (In this file the choice between a OfferedResources vs. Allocation seems arbitrary)
Thoughts?
- Jiang Yan Xu
On Jan. 23, 2017, 6:31 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55868/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2017, 6:31 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This was necessary to greatly simplify the changes needed to the
> allocator tests as we introduce support for multi-role frameworks.
>
> The main improvement here is to establish and use equality on the
> `Allocation` struct, which makes the tests more readable and avoids
> the manual probing of the allocation structure across all the tests.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5
>
> Diff: https://reviews.apache.org/r/55868/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55868: Cleanups to the allocator tests.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55868/#review163566
-----------------------------------------------------------
Ship it!
Ship It!
- Michael Park
On Jan. 23, 2017, 6:31 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55868/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2017, 6:31 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This was necessary to greatly simplify the changes needed to the
> allocator tests as we introduce support for multi-role frameworks.
>
> The main improvement here is to establish and use equality on the
> `Allocation` struct, which makes the tests more readable and avoids
> the manual probing of the allocation structure across all the tests.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5
>
> Diff: https://reviews.apache.org/r/55868/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55868: Cleanups to the allocator tests.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55868/#review162769
-----------------------------------------------------------
Patch looks great!
Reviews applied: [54842, 54836, 55825, 55826, 55827, 55829, 55828, 55863, 55866, 55824, 55868]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Jan. 24, 2017, 2:31 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55868/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2017, 2:31 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This was necessary to greatly simplify the changes needed to the
> allocator tests as we introduce support for multi-role frameworks.
>
> The main improvement here is to establish and use equality on the
> `Allocation` struct, which makes the tests more readable and avoids
> the manual probing of the allocation structure across all the tests.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5
>
> Diff: https://reviews.apache.org/r/55868/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>