Re: Review Request 55967: Update the allocator unit tests to reflect MULTI_ROLE support.

[Benjamin Mahler]

> On Jan. 31, 2017, 7:25 a.m., Michael Park wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 3414-3417
> > 
> >
> > 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
> > 
> >
> > 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.

[Benjamin Mahler]

> On Jan. 28, 2017, 1:16 p.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 3942
> > 
> >
> > 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
> > 
> >
> > 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.

[Michael Park]

---
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)


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)


Should just use `foreachpair` twice here I think. A few places below as 
well.



src/tests/hierarchical_allocator_tests.cpp (lines 3728 - 3730)


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)


`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)


`{` 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.

[Guangya Liu]

---
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)


Need call `allocation.allocate("*");` here to make sure the agent can be 
added successfully.



src/tests/hierarchical_allocator_tests.cpp (line 3934)


Need call `allocation.allocate("*");` here to make sure the agent can be 
added successfully.



src/tests/hierarchical_allocator_tests.cpp (line 3934)


Need call `allocation.allocate("*");` here to make sure the agent can be 
added successfully.



src/tests/hierarchical_allocator_tests.cpp (line 4084)


Need call `allocation.allocate("*");` here to make sure the agent can be 
added successfully.



src/tests/hierarchical_allocator_tests.cpp (lines 4188 - 4189)


Why moving this here?



src/tests/hierarchical_allocator_tests.cpp (line 4243)


Need call `allocation.allocate("*");` here to make sure the agent can be 
added successfully.


- Guangya Liu


On 一月 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 一月 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
> 
>