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/26 00:46:42 UTC
Review Request 55967: Update the allocator unit tests to reflect
MULTI_ROLE support.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55967/
-----------------------------------------------------------
Review request for mesos, Guangya Liu and Michael Park.
Repository: mesos
Description
-------
Update the allocator unit tests to reflect MULTI_ROLE support.
Diffs
-----
src/tests/hierarchical_allocator_tests.cpp 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5
Diff: https://reviews.apache.org/r/55967/diff/
Testing
-------
make check
note that the rest of the tests do not pass until after my subsequent patches
Thanks,
Benjamin Mahler
Re: Review Request 55967: Update the allocator unit tests to reflect
MULTI_ROLE support.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Jan. 31, 2017, 7:25 a.m., Michael Park wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 3414-3417
> > <https://reviews.apache.org/r/55967/diff/1/?file=1616016#file1616016line3414>
> >
> > Should just use `foreachpair` twice here I think. A few places below as well.
Yeah we could, just seemed a bit cleaner to avoid introducing another resources variable.
> On Jan. 31, 2017, 7:25 a.m., Michael Park wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 3737-3739
> > <https://reviews.apache.org/r/55967/diff/1/?file=1616016#file1616016line3737>
> >
> > Operators should be placed in the previous lines I think?
> >
> > I think maybe it'd be better if we were to just pull out the `allocatedTaskResources`?
> >
> > ```cpp
> > AllocatedResources allocatedTaskResources(task.resources(), "role1");
> >
> > expected = Allocation::create(
> > framework2.id(),
> > {{"role1",
> > {{slave.id(), updated.get() - allocatedTaskResources + volume}}}});
> > ```
Sounds good! Turns out I don't need to pull it out since now the adjustment of the operation mutates it rather than returning a copy.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55967/#review163632
-----------------------------------------------------------
On Jan. 26, 2017, 12:46 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55967/
> -----------------------------------------------------------
>
> (Updated Jan. 26, 2017, 12:46 a.m.)
>
>
> Review request for mesos, Guangya Liu and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Update the allocator unit tests to reflect MULTI_ROLE support.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5
>
> Diff: https://reviews.apache.org/r/55967/diff/
>
>
> Testing
> -------
>
> make check
>
> note that the rest of the tests do not pass until after my subsequent patches
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55967: Update the allocator unit tests to reflect
MULTI_ROLE support.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55967/#review163632
-----------------------------------------------------------
Fix it, then Ship it!
src/tests/hierarchical_allocator_tests.cpp (lines 944 - 945)
<https://reviews.apache.org/r/55967/#comment235102>
This seems like an odd test. Unforunate that we can't seem to use `Allocation` because of this.
src/tests/hierarchical_allocator_tests.cpp (lines 3408 - 3411)
<https://reviews.apache.org/r/55967/#comment235101>
Should just use `foreachpair` twice here I think. A few places below as well.
src/tests/hierarchical_allocator_tests.cpp (lines 3728 - 3730)
<https://reviews.apache.org/r/55967/#comment235103>
Operators should be placed in the previous lines I think?
I think maybe it'd be better if we were to just pull out the `allocatedTaskResources`?
```cpp
AllocatedResources allocatedTaskResources(task.resources(), "role1");
expected = Allocation::create(
framework2.id(),
{{"role1",
{{slave.id(), updated.get() - allocatedTaskResources + volume}}}});
```
src/tests/hierarchical_allocator_tests.cpp (lines 3810 - 3813)
<https://reviews.apache.org/r/55967/#comment235104>
`s/allocation/allocated/` since we have an `Allocation` type now? Also, could you have used `AllocatedResources` here?
src/tests/hierarchical_allocator_tests.cpp (line 4180)
<https://reviews.apache.org/r/55967/#comment235106>
`{` on the newline.
- Michael Park
On Jan. 25, 2017, 4:46 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55967/
> -----------------------------------------------------------
>
> (Updated Jan. 25, 2017, 4:46 p.m.)
>
>
> Review request for mesos, Guangya Liu and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Update the allocator unit tests to reflect MULTI_ROLE support.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5
>
> Diff: https://reviews.apache.org/r/55967/diff/
>
>
> Testing
> -------
>
> make check
>
> note that the rest of the tests do not pass until after my subsequent patches
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55967: Update the allocator unit tests to reflect
MULTI_ROLE support.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Jan. 28, 2017, 1:16 p.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 3942
> > <https://reviews.apache.org/r/55967/diff/1/?file=1616016#file1616016line3942>
> >
> > Need call `allocation.allocate("*");` here to make sure the agent can be added successfully.
Good catch Guangya! Unfortunately I didn't run the benchmark, I wish they ran in a reasonable amount of time :(
> On Jan. 28, 2017, 1:16 p.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 4093
> > <https://reviews.apache.org/r/55967/diff/1/?file=1616016#file1616016line4093>
> >
> > Need call `allocation.allocate("*");` here to make sure the agent can be added successfully.
Thanks! Needs to be role1 though FWICT.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55967/#review163398
-----------------------------------------------------------
On Jan. 26, 2017, 12:46 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55967/
> -----------------------------------------------------------
>
> (Updated Jan. 26, 2017, 12:46 a.m.)
>
>
> Review request for mesos, Guangya Liu and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Update the allocator unit tests to reflect MULTI_ROLE support.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5
>
> Diff: https://reviews.apache.org/r/55967/diff/
>
>
> Testing
> -------
>
> make check
>
> note that the rest of the tests do not pass until after my subsequent patches
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55967: Update the allocator unit tests to reflect
MULTI_ROLE support.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55967/#review163398
-----------------------------------------------------------
src/tests/hierarchical_allocator_tests.cpp (line 3933)
<https://reviews.apache.org/r/55967/#comment234859>
Need call `allocation.allocate("*");` here to make sure the agent can be added successfully.
src/tests/hierarchical_allocator_tests.cpp (line 3934)
<https://reviews.apache.org/r/55967/#comment234855>
Need call `allocation.allocate("*");` here to make sure the agent can be added successfully.
src/tests/hierarchical_allocator_tests.cpp (line 3934)
<https://reviews.apache.org/r/55967/#comment234856>
Need call `allocation.allocate("*");` here to make sure the agent can be added successfully.
src/tests/hierarchical_allocator_tests.cpp (line 4084)
<https://reviews.apache.org/r/55967/#comment234858>
Need call `allocation.allocate("*");` here to make sure the agent can be added successfully.
src/tests/hierarchical_allocator_tests.cpp (lines 4188 - 4189)
<https://reviews.apache.org/r/55967/#comment234846>
Why moving this here?
src/tests/hierarchical_allocator_tests.cpp (line 4243)
<https://reviews.apache.org/r/55967/#comment234857>
Need call `allocation.allocate("*");` here to make sure the agent can be added successfully.
- Guangya Liu
On \u4e00\u6708 26, 2017, 12:46 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55967/
> -----------------------------------------------------------
>
> (Updated \u4e00\u6708 26, 2017, 12:46 a.m.)
>
>
> Review request for mesos, Guangya Liu and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Update the allocator unit tests to reflect MULTI_ROLE support.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5
>
> Diff: https://reviews.apache.org/r/55967/diff/
>
>
> Testing
> -------
>
> make check
>
> note that the rest of the tests do not pass until after my subsequent patches
>
>
> Thanks,
>
> Benjamin Mahler
>
>