> On April 8, 2015, 8:16 p.m., Jie Yu wrote:
> > src/common/resources.cpp, lines 450-459
> > <https://reviews.apache.org/r/32140/diff/5/?file=920081#file920081line450>
> >
> >     The semantics of this function becomes a little weired now. For 
> > example, for a resource that has `role == "*"` and has reservation set, 
> > `isReserved(resource, "*")` is going to return `true`? Given that 
> > 'resource' is invalid, we should return a `false` in that case?
> 
> Alexander Rukletsov wrote:
>     Can we have a resource with `role == "*"` and reservations set?
> 
> Alexander Rukletsov wrote:
>     Excuse my premature comment earlier, I'm slowly starting to understand 
> what's going on here. It looks like in the case Jie describes, the function 
> returns `true` indeed. Which is invalid by now, but _may_ become valid in the 
> future. On the other side, I'm not sure that returning `false` in this case 
> is an alternative: it will read as unreserved resource, not an invalid 
> resource. We can wrap the return value in `Try` but I prefer the way it's 
> done now.

Hey guys, this predicate, as with other predicates assume that the resources 
have already been validated.
The following note can be found at the top of the predicate declarations in the 
header.

```
// NOTE: The following predicate functions assume that the given
// resource is validated.
```

Is it still weird with this assumption? If it was just that the note is 
difficult to find, I could maybe put the note as a comment on every predicate?


- Michael


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


On April 22, 2015, 8:36 p.m., Michael Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32140/
> -----------------------------------------------------------
> 
> (Updated April 22, 2015, 8:36 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
> 
> 
> Bugs: MESOS-2476
>     https://issues.apache.org/jira/browse/MESOS-2476
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> `Resource::ReservationInfo` was introduced in 
> [r32139](https://reviews.apache.org/r/32139). We need to consider it in our 
> `Resources` class implementation.
> 
> ### `Resources::flatten`
> 
> `flatten` is used as the utility function to cross reservation boundaries 
> without affecting the given resources. Since the reservation is now specified 
> by the (`role`, `reservation`) pair, `flatten` needs to consider 
> `ReservationInfo` as well.
> 
> ### `Resources::validate`
> 
> If `role == "*"`, then `reservation` field must not be set.
> 
> ### `Resources` comparators
> 
> `operator==`, `addable` and `substractable` need to test for 
> `ReservationInfo` as well.
> 
> 
> Diffs
> -----
> 
>   include/mesos/resources.hpp 56affd45e1e71e96ff5778b43271f81b9b9a9e4d 
>   src/common/resources.cpp 2c99b6884d7296099e19e2e3182cbe11b5e1e059 
>   src/tests/mesos.hpp 4294e28f904161b473c0cfa7feea4eaa4e7c97e3 
>   src/tests/resources_tests.cpp 7e0ad98c3366f647f190363a0e6b576dbfc7d415 
> 
> Diff: https://reviews.apache.org/r/32140/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Reply via email to