mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Park" <mcyp...@gmail.com>
Subject Re: Review Request 32140: Enabled 'Resources' to handle 'Resource::ReservationInfo'.
Date Wed, 22 Apr 2015 22:01:44 GMT


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


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