> On March 25, 2015, 5:07 p.m., Timothy Chen wrote:
> > include/mesos/resources.hpp, line 182
> > <https://reviews.apache.org/r/32140/diff/4/?file=901855#file901855line182>
> >
> > I'm not sure the following comment adds any value?
> >
> > "If the optional ReservationInfo is given, the resource's
> > // 'reservation' field is set. Otherwise, the resource's
> > // 'reservation' field is not set."
> >
> > It sounds like it's just stating the obvious.
> >
> > What do you think? I think we should just remove it.
Ah, sorry. Having re-read it, it does sound obvious. What I was trying to
capture is: "If the optional ReservationInfo is given, the resource's
`reservation` field is set. Otherwise, the resouce's `reservation` field is
*cleared*."
The important distinction being: *untouched* and *cleared*.
For example, if we have a `Resources resources` object:
```javascript
[
{
name : "cpus",
value : 2,
role : "ads"
reservation : { principal: "foo" },
...
}
]
```
```
Resources flattened = resources.flatten();
```
`flattened` will be:
```javascript
[
{
name : "cpus",
value : 2,
role : "*",
reservation : None,
...
}
]
```
> On March 25, 2015, 5:07 p.m., Timothy Chen wrote:
> > src/common/resources.cpp, line 101
> > <https://reviews.apache.org/r/32140/diff/4/?file=901856#file901856line101>
> >
> > Why is this check necessary if we're already checking it above?
Those are actually not the same checks.
The general pattern I followed is:
```
if (left.has_field() != right.has_field()) {
// Either left is set and right isn't, or right is set and left isn't.
return false;
}
if (left.has_field() && left.field() != right.field()) {
// Both are set, but the values differ.
return false;
}
// Both are set and the values are equal, or neither or set.
```
It looks awkward because in the second `if` statement we only check
`left.has_field()` since `right.has_field()` must be the same answer. I'm not a
fan of the asymmetry but I followed the existing pattern.
> On March 25, 2015, 5:07 p.m., Timothy Chen wrote:
> > src/common/resources.cpp, line 407
> > <https://reviews.apache.org/r/32140/diff/4/?file=901856#file901856line407>
> >
> > Why check framework Id? Shouldn't the first 2 checks be enough?
Now that we've taken out `framework_id`, we just check for
```
if (resource.role() == "*" && resource.has_reservation()) {
return Error(...);
}
```
> On March 25, 2015, 5:07 p.m., Timothy Chen wrote:
> > src/common/resources.cpp, line 466
> > <https://reviews.apache.org/r/32140/diff/4/?file=901856#file901856line466>
> >
> > As we already checked for * don't have reservation, seems like we
> > should just check for has_reservation then?
Both checks are important here. We do invalidate the `role == "*" &&
has_reservation` case, but simply checking for `!has_reservation` means that
static reservations would be considered __unreserved__.
```
unreserved : role == "*" && !has_reservation
statically reserved : role == R && !has_reservation where R != "*"
(invalid) : role == "*" && has_reservation
dynamically reserved : role == R && has_reservation where R != "*".
```
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review77752
-----------------------------------------------------------
On April 15, 2015, 4:12 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32140/
> -----------------------------------------------------------
>
> (Updated April 15, 2015, 4:12 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 0e98572a62ae05437bd2bc800c370ad1a0c43751
> src/tests/resources_tests.cpp 7e0ad98c3366f647f190363a0e6b576dbfc7d415
>
> Diff: https://reviews.apache.org/r/32140/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
