mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jie Yu" <yujie....@gmail.com>
Subject Re: Review Request 32140: Enabled 'Resources' to handle 'Resource::ReservationInfo'.
Date Sun, 03 May 2015 20:10:31 GMT


> 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`.
> 
> Michael Park wrote:
>     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.
> 
> Alexander Rukletsov wrote:
>     The problem with `has_*()` checks is that if you specify the value which is equal
to the default, you check will fail and messages will be considered different.
> 
> Michael Park wrote:
>     But that's exactly what we want. The following 2 `Resources` objects ought to be
considered different.
>     
>     ```js
>     // statically reserved for "ads" role.
>     {
>       name        : "cpus",
>       value       : 2,
>       role        : "ads",
>       reservation : None,
>       ...
>     }
>     ```
>     
>     ```js
>     // dynamically reserved for "ads" role by "" principal.
>     {
>       name        : "cpus",
>       value       : 2,
>       role        : "ads",
>       reservation : { prinicipal: "", },
>       ...
>     }
>     ```
>     
>     Without the `has_*()` checks, the above messages are incorrectly treated equal. It
may be the case that we explicitly disallow `""` `principal`s, but in general that's not the
case. My main point is that omitting the `has_*()` checks don't work in the general case.
> 
> Alexander Rukletsov wrote:
>     Michael, that's a totally valid point, but let's document it (because it differs
from a default way we comapre protobufs now) and mark the issue fixed.

Michael, I committed this patch. Follow up with another patch if you want to add some document
as Alex suggested.


- Jie


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


On April 27, 2015, 5:28 p.m., Michael Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32140/
> -----------------------------------------------------------
> 
> (Updated April 27, 2015, 5:28 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 19db71217f0a3f1ab17a6fd4408f8251410d731d 
>   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