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

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

Reply via email to