mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 44334: Cleaned up empty hashmaps from allocator tests.
Date Wed, 13 Apr 2016 08:19:36 GMT


> 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<SlaveID, Resources>()" 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message