> On March 18, 2015, 12:27 a.m., Alexander Rukletsov wrote:
> > src/common/resources.cpp, lines 69-74
> > <https://reviews.apache.org/r/32140/diff/1/?file=897349#file897349line69>
> >
> > Not yours, but resently, Vinod did a cleanup in equivalence operators
> > for our proto messages in `type_utils.{hpp|cpp}`:
> > https://reviews.apache.org/r/31904/. I think we should do the same here for
> > consistency (most probably in a separate RR). Also, one more point to
> > extract these out to `type_utils`.
Hm, I actually missed that review request. That pattern of comparisons don't
always work... doesn't seem like good practice to me.
In particular, it doesn't work when "unset" and the "default" message mean
different things. Consider our `ReservationInfo` example:
The (role, reservation) pairs:
- ("role", None) means statically reserved for "role"
- ("role", { framework_id: None }) means dynamically reserved for "role"
If we compare by `left.reservation() == right.reservation()`, the 2 states
above are considered equal because `left.reservation()` returns `{
framework_id: None }`, according to [protobuf
documentation](https://developers.google.com/protocol-buffers/docs/cpptutorial):
> optional: the field may or may not be set. If an optional field value isn't
> set, a default value is used. For simple types, you can specify your own
> default value, as we've done for the phone number type in the example.
> Otherwise, a system default is used: zero for numeric types, the empty string
> for strings, false for bools. **For embedded messages, the default value is
> always the "default instance" or "prototype" of the message, which has none
> of its fields set. Calling the accessor to get the value of an optional (or
> required) field which has not been explicitly set always returns that field's
> default value.**
We actually do need to check for `has_` conditions to get it right.
```
left.has_reservation() == right.has_reservation() && (!left.has_reservation()
|| left.reservation() == right.reservation());
```
`DiskInfo` is another embdedded message that has all optional fields similar to
`ReservationInfo`, except its "unset" is equivalent to "default" message. So
doing `left.disk() == right.disk()` works.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review76752
-----------------------------------------------------------
On March 18, 2015, 7:44 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32140/
> -----------------------------------------------------------
>
> (Updated March 18, 2015, 7:44 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 da6d48871a0061d8bbf5e681dd6e7edac659d812
> src/common/resources.cpp 2c99b6884d7296099e19e2e3182cbe11b5e1e059
> src/tests/mesos.hpp 45e35204d1aa876fa0c871acf0f21afcd5ababe8
> src/tests/resources_tests.cpp 7e0ad98c3366f647f190363a0e6b576dbfc7d415
>
> Diff: https://reviews.apache.org/r/32140/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
