Re: Review Request 44334: Cleaned up empty hashmaps from allocator tests.

[Benjamin Bannier]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44334/#review128631
---


Fix it, then Ship it!





src/tests/hierarchical_allocator_tests.cpp (line 178)


What about this one?


- Benjamin Bannier


On March 3, 2016, 2 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44334/
> ---
> 
> (Updated March 3, 2016, 2 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/44334/diff/
> 
> 
> Testing
> ---
> 
> Tested as a chain in https://reviews.apache.org/r/44336/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44334: Cleaned up empty hashmaps from allocator tests.

[Benjamin Bannier]

> On March 4, 2016, 12:59 p.m., Bernd Mathiske wrote:
> > "{}" is short, but cryptic. It is unclear what kind of entity is being 
> > passed here. "EMPTY" was not any better. "hashmap()" at 
> > least revealed the type which hinted a little at the presumable purpose. A 
> > better identifier than "EMPTY" would beat all of the above. Maybe 
> > "NO_USED_RESOURCES"?
> > 
> > Could the parameter of the methods in question be supplied with a default 
> > value? Then no corresponding argument at all would appear in these tests.
> 
> Alexander Rukletsov wrote:
> I'm a bit reluctant to adding a default value, because these are public 
> functions from an `Allocator` interface. In previous versions of the patch I 
> introduced verbose constants like `NO_USED_RESOURCES` and `NO_ALLOCATIONS` 
> but we opted for more cryptic but shorter `{}`. Functions `addSlave()` and 
> `addFramework()` are crucial to allocator tests, hence motivating people to 
> look at their signatures is not a bad idea : ).

@Bernd: I am confused why we'd want to be specific about the type of these 
default-constructed function call arguments here. I can understand that 
avoiding `auto` on an assignments LHS helps the reader anticipate e.g., the 
interface satisfied by some variable's type, but for arguments we do nothing 
with the value.

Moreover, in this case we construct empty sets and at least to me `{}` is a 
very intuitive notation for that.


- Benjamin


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44334/#review122058
---


On March 3, 2016, 2 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44334/
> ---
> 
> (Updated March 3, 2016, 2 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/44334/diff/
> 
> 
> Testing
> ---
> 
> Tested as a chain in https://reviews.apache.org/r/44336/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44334: Cleaned up empty hashmaps from allocator tests.

[Alexander Rukletsov]

> On March 4, 2016, 11:59 a.m., Bernd Mathiske wrote:
> > "{}" is short, but cryptic. It is unclear what kind of entity is being 
> > passed here. "EMPTY" was not any better. "hashmap()" at 
> > least revealed the type which hinted a little at the presumable purpose. A 
> > better identifier than "EMPTY" would beat all of the above. Maybe 
> > "NO_USED_RESOURCES"?
> > 
> > Could the parameter of the methods in question be supplied with a default 
> > value? Then no corresponding argument at all would appear in these tests.

I'm a bit reluctant to adding a default value, because these are public 
functions from an `Allocator` interface. In previous versions of the patch I 
introduced verbose constants like `NO_USED_RESOURCES` and `NO_ALLOCATIONS` but 
we opted for more cryptic but shorter `{}`. Functions `addSlave()` and 
`addFramework()` are crucial to allocator tests, hence motivating people to 
look at their signatures is not a bad idea : ).


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44334/#review122058
---


On March 3, 2016, 1 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44334/
> ---
> 
> (Updated March 3, 2016, 1 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/44334/diff/
> 
> 
> Testing
> ---
> 
> Tested as a chain in https://reviews.apache.org/r/44336/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44334: Cleaned up empty hashmaps from allocator tests.

[Bernd Mathiske]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44334/#review122058
---



"{}" is short, but cryptic. It is unclear what kind of entity is being passed 
here. "EMPTY" was not any better. "hashmap()" at least 
revealed the type which hinted a little at the presumable purpose. A better 
identifier than "EMPTY" would beat all of the above. Maybe "NO_USED_RESOURCES"?

Could the parameter of the methods in question be supplied with a default 
value? Then no corresponding argument at all would appear in these tests.

- Bernd Mathiske


On March 3, 2016, 5 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44334/
> ---
> 
> (Updated March 3, 2016, 5 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/44334/diff/
> 
> 
> Testing
> ---
> 
> Tested as a chain in https://reviews.apache.org/r/44336/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>