mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexander Rukletsov" <ruklet...@gmail.com>
Subject Re: Review Request 32140: Enable 'Resources' to handle 'Resource::ReservationInfo'.
Date Wed, 25 Mar 2015 19:43:56 GMT


> On March 18, 2015, 12:27 a.m., Alexander Rukletsov wrote:
> > src/common/resources.cpp, lines 44-49
> > <https://reviews.apache.org/r/32140/diff/1/?file=897349#file897349line44>
> >
> >     Why these operatos (also those that were here before) are defined here and not
in `include/mesos/type_utils.{hpp|cpp}`?
> 
> Michael Park wrote:
>     I defined them here to keep consistency with the operators for `DiskInfo`. I think
it's because we don't really need these comparators outside of this file. @jieyu: What do
you think?
> 
> Jie Yu wrote:
>     Mpark, you are right. The DiskInfo == operator is only used in resources.cpp currently.
Also, we usually keep the operator close to the corresponding class. For example, we put operator
== of Value::Scalar in src/common/values.cpp rather than type_utils.hpp.

I prefer to keep similar stuff in similar places for the principal of least surprise, but
I let you guys decide. I don't think we can appeal to consistency here, since one case (`DiskInfo`
operators) is not a statistic. Feel free to drop.


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

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.


- Alexander


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


On March 20, 2015, 10:36 p.m., Michael Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32140/
> -----------------------------------------------------------
> 
> (Updated March 20, 2015, 10: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 45e35204d1aa876fa0c871acf0f21afcd5ababe8 
>   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