You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Michael Park <mc...@gmail.com> on 2015/03/17 04:20:01 UTC
Review Request 32140: Enable 'Resources' to handle
'Resource::ReservationInfo'.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/
-----------------------------------------------------------
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
Diff: https://reviews.apache.org/r/32140/diff/
Testing
-------
make check
Thanks,
Michael Park
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Jie Yu <yu...@gmail.com>.
> 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
>
>
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Michael Park <mc...@gmail.com>.
> 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.
>
> Jie Yu wrote:
> Michael, I committed this patch. Follow up with another patch if you want to add some document as Alex suggested.
I've created [MESOS-2710](https://issues.apache.org/jira/browse/MESOS-2710) to keep track of this issue.
- Michael
-----------------------------------------------------------
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
>
>
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Michael Park <mc...@gmail.com>.
> 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.
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.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review76752
-----------------------------------------------------------
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
>
>
Re: Review Request 32140: Enable 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> 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
>
>
Re: Review Request 32140: Enable 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Jie Yu <yu...@gmail.com>.
> 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?
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.
- Jie
-----------------------------------------------------------
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
>
>
Re: Review Request 32140: Enable 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Michael Park <mc...@gmail.com>.
> 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
>
>
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> 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.
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.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review76752
-----------------------------------------------------------
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
>
>
Re: Review Request 32140: Enable 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Michael Park <mc...@gmail.com>.
> 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}`?
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?
> On March 18, 2015, 12:27 a.m., Alexander Rukletsov wrote:
> > src/common/resources.cpp, line 156
> > <https://reviews.apache.org/r/32140/diff/1/?file=897349#file897349line156>
> >
> > Can we subtract resources reserved for different frameworks?
Nope. Good find, this was a bug I fixed earlier that I found while writing tests.
> On March 18, 2015, 12:27 a.m., Alexander Rukletsov wrote:
> > src/common/resources.cpp, line 394
> > <https://reviews.apache.org/r/32140/diff/1/?file=897349#file897349line394>
> >
> > I would put accent on reservation: "Invalid reservation ..."
Done.
> On March 18, 2015, 12:27 a.m., Alexander Rukletsov wrote:
> > src/common/resources.cpp, lines 437-439
> > <https://reviews.apache.org/r/32140/diff/1/?file=897349#file897349line437>
> >
> > This checks whether a resource is reserved for a role. Do we need to have an overload for framework reservations? Or add optional `FrameworkID` arg?
That will be included in a subsequent task, [MESOS-2490](https://issues.apache.org/jira/browse/MESOS-2490), where we "teach the allocator to offer the resources to the particular framework rather than the role".
I've scoped out the current task to simply to allow the framework to send the Reserve operation and update the allocator resources.
- 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
>
>
Re: Review Request 32140: Enable 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review76752
-----------------------------------------------------------
src/common/resources.cpp
<https://reviews.apache.org/r/32140/#comment124414>
Why these operatos (also those that were here before) are defined here and not in `include/mesos/type_utils.{hpp|cpp}`?
src/common/resources.cpp
<https://reviews.apache.org/r/32140/#comment124420>
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`.
src/common/resources.cpp
<https://reviews.apache.org/r/32140/#comment124423>
Can we subtract resources reserved for different frameworks?
src/common/resources.cpp
<https://reviews.apache.org/r/32140/#comment124487>
I would put accent on reservation: "Invalid reservation ..."
src/common/resources.cpp
<https://reviews.apache.org/r/32140/#comment124488>
This checks whether a resource is reserved for a role. Do we need to have an overload for framework reservations? Or add optional `FrameworkID` arg?
- Alexander Rukletsov
On March 17, 2015, 4:06 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32140/
> -----------------------------------------------------------
>
> (Updated March 17, 2015, 4:06 a.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
>
> Diff: https://reviews.apache.org/r/32140/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On April 15, 2015, 5:36 p.m., Alexander Rukletsov wrote:
> > src/common/resources.cpp, lines 93-105
> > <https://reviews.apache.org/r/32140/diff/5/?file=920081#file920081line93>
> >
> > Can we replace it by
> > ```
> > if !addable(left, right) {
> > return false;
> > }
> > ```
> > ? Or even add `addable()` check to the chain of checks below?
>
> Michael Park wrote:
> Each of `operator==`, `addable` and `subtractable` has subtle differences.
>
> `operator==` and `addable` in specific are different in that equal persistent volumes (same persistence ID) are not addable.
> Here's Jie's comment:
>
> ```
> // TODO(jieyu): Even if two Resource objects with DiskInfo have the
> // same persistence ID, they cannot be added together. In fact, this
> // shouldn't happen if we do not add resources from different
> // namespaces (e.g., across slave). Consider adding a warning.
> ```
Got it.
> On April 15, 2015, 5:36 p.m., Alexander Rukletsov wrote:
> > src/tests/mesos.hpp, lines 376-378
> > <https://reviews.apache.org/r/32140/diff/5/?file=920082#file920082line376>
> >
> > Why not putting the definition into `tests/mesos.cpp`? Here and below.
>
> Michael Park wrote:
> I kept it consistent with other utility functions that live in this file: `createTask`, `createDiskInfo`, `createPersistentVolume`. I think we can fix this as a batch in a separate patch.
Mind creating a JIRA?
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review80201
-----------------------------------------------------------
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
>
>
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On April 15, 2015, 5:36 p.m., Alexander Rukletsov wrote:
> > src/common/resources.cpp, lines 455-464
> > <https://reviews.apache.org/r/32140/diff/5/?file=920081#file920081line455>
> >
> > Agree with Jie, I spent some time trying to understand what's going on here and what the implications are.
> >
> > * `.has_reservation()` means a resource is dynamically reserved;
> > * `.role() != "*"` means a resource is reserved for a role.
> >
> > Currently we do not allow reservations for "*" role, this is validated above. But what is the semantics of `isUnreserved()` function? It used to be "a resource belongs to a default role". Let's write a comment to document our intention here, mentioning all assumptions that we currently make.
>
> Michael Park wrote:
> I've added the valid states of the (role, reservation) pair which I think help in reasoning through the logic of the predicates. Do you think this is sufficient?
Yep, looks good. Thanks!
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review80201
-----------------------------------------------------------
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
>
>
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Michael Park <mc...@gmail.com>.
> On April 15, 2015, 5:36 p.m., Alexander Rukletsov wrote:
> > src/tests/mesos.hpp, lines 376-378
> > <https://reviews.apache.org/r/32140/diff/5/?file=920082#file920082line376>
> >
> > Why not putting the definition into `tests/mesos.cpp`? Here and below.
I kept it consistent with other utility functions that live in this file: `createTask`, `createDiskInfo`, `createPersistentVolume`. I think we can fix this as a batch in a separate patch.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review80201
-----------------------------------------------------------
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
>
>
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Michael Park <mc...@gmail.com>.
> On April 15, 2015, 5:36 p.m., Alexander Rukletsov wrote:
> > src/tests/mesos.hpp, lines 376-378
> > <https://reviews.apache.org/r/32140/diff/5/?file=920082#file920082line376>
> >
> > Why not putting the definition into `tests/mesos.cpp`? Here and below.
>
> Michael Park wrote:
> I kept it consistent with other utility functions that live in this file: `createTask`, `createDiskInfo`, `createPersistentVolume`. I think we can fix this as a batch in a separate patch.
>
> Alexander Rukletsov wrote:
> Mind creating a JIRA?
Added [MESOS-2658](https://issues.apache.org/jira/browse/MESOS-2658) as a subtask of [MESOS-1622](https://issues.apache.org/jira/browse/MESOS-1622).
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review80201
-----------------------------------------------------------
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
>
>
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Michael Park <mc...@gmail.com>.
> On April 15, 2015, 5:36 p.m., Alexander Rukletsov wrote:
> > src/common/resources.cpp, lines 455-464
> > <https://reviews.apache.org/r/32140/diff/5/?file=920081#file920081line455>
> >
> > Agree with Jie, I spent some time trying to understand what's going on here and what the implications are.
> >
> > * `.has_reservation()` means a resource is dynamically reserved;
> > * `.role() != "*"` means a resource is reserved for a role.
> >
> > Currently we do not allow reservations for "*" role, this is validated above. But what is the semantics of `isUnreserved()` function? It used to be "a resource belongs to a default role". Let's write a comment to document our intention here, mentioning all assumptions that we currently make.
I've added the valid states of the (role, reservation) pair which I think help in reasoning through the logic of the predicates. Do you think this is sufficient?
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review80201
-----------------------------------------------------------
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
>
>
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On April 15, 2015, 5:36 p.m., Alexander Rukletsov wrote:
> > src/tests/mesos.hpp, lines 376-378
> > <https://reviews.apache.org/r/32140/diff/5/?file=920082#file920082line376>
> >
> > Why not putting the definition into `tests/mesos.cpp`? Here and below.
>
> Michael Park wrote:
> I kept it consistent with other utility functions that live in this file: `createTask`, `createDiskInfo`, `createPersistentVolume`. I think we can fix this as a batch in a separate patch.
>
> Alexander Rukletsov wrote:
> Mind creating a JIRA?
>
> Michael Park wrote:
> Added [MESOS-2658](https://issues.apache.org/jira/browse/MESOS-2658) as a subtask of [MESOS-1622](https://issues.apache.org/jira/browse/MESOS-1622).
Awesome!
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review80201
-----------------------------------------------------------
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
>
>
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Michael Park <mc...@gmail.com>.
> On April 15, 2015, 5:36 p.m., Alexander Rukletsov wrote:
> > src/common/resources.cpp, lines 93-105
> > <https://reviews.apache.org/r/32140/diff/5/?file=920081#file920081line93>
> >
> > Can we replace it by
> > ```
> > if !addable(left, right) {
> > return false;
> > }
> > ```
> > ? Or even add `addable()` check to the chain of checks below?
Each of `operator==`, `addable` and `subtractable` has subtle differences.
`operator==` and `addable` in specific are different in that equal persistent volumes (same persistence ID) are not addable.
Here's Jie's comment:
```
// TODO(jieyu): Even if two Resource objects with DiskInfo have the
// same persistence ID, they cannot be added together. In fact, this
// shouldn't happen if we do not add resources from different
// namespaces (e.g., across slave). Consider adding a warning.
```
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review80201
-----------------------------------------------------------
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
>
>
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review80201
-----------------------------------------------------------
src/common/resources.cpp
<https://reviews.apache.org/r/32140/#comment129980>
Can we replace it by
```
if !addable(left, right) {
return false;
}
```
? Or even add `addable()` check to the chain of checks below?
src/common/resources.cpp
<https://reviews.apache.org/r/32140/#comment129981>
Remove period at the end.
src/common/resources.cpp
<https://reviews.apache.org/r/32140/#comment129988>
Agree with Jie, I spent some time trying to understand what's going on here and what the implications are.
* `.has_reservation()` means a resource is dynamically reserved;
* `.role() != "*"` means a resource is reserved for a role.
Currently we do not allow reservations for "*" role, this is validated above. But what is the semantics of `isUnreserved()` function? It used to be "a resource belongs to a default role". Let's write a comment to document our intention here, mentioning all assumptions that we currently make.
src/tests/mesos.hpp
<https://reviews.apache.org/r/32140/#comment129998>
Why not putting the definition into `tests/mesos.cpp`? Here and below.
- Alexander Rukletsov
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
>
>
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Michael Park <mc...@gmail.com>.
> On April 22, 2015, 12:14 p.m., Alexander Rukletsov wrote:
> > src/common/resources.cpp, lines 44-49
> > <https://reviews.apache.org/r/32140/diff/5/?file=920081#file920081line44>
> >
> > Won't this be equivalent to the auto-generated version?
I'm not sure what auto-generated version you're referring to? Protobuf doesn't auto-generate `operator==`.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review81143
-----------------------------------------------------------
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
>
>
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review81143
-----------------------------------------------------------
src/common/resources.cpp
<https://reviews.apache.org/r/32140/#comment131406>
Won't this be equivalent to the auto-generated version?
- Alexander Rukletsov
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
>
>
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
Added the valid states of (role, reservation) pair of Resource objects to improve documentation on what the Resource predicates represent.
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 (updated)
-----
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
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/
-----------------------------------------------------------
(Updated April 23, 2015, 8:25 p.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
Changes
-------
Addressed some of Jie's comments.
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 (updated)
-----
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
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Michael Park <mc...@gmail.com>.
> On April 23, 2015, 5:35 p.m., Jie Yu wrote:
> > src/common/resources.cpp, lines 445-449
> > <https://reviews.apache.org/r/32140/diff/6/?file=939885#file939885line445>
> >
> > See my comments in https://reviews.apache.org/r/32150/
> >
> > Can we move this to master validation? Thoughts?
Synced with Jie on IRC regarding this topic. We agreed that `Resources::validate` needs to capture the invariant of the `Resource` object which means it needs to invalidate the `role == "*" && has_reservation()` state. This invariant is required for all the predicates as well as functions such as `reserved()` and `unreserved()` to have well-defined behavior.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review81342
-----------------------------------------------------------
On April 23, 2015, 8:25 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32140/
> -----------------------------------------------------------
>
> (Updated April 23, 2015, 8:25 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
>
>
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Michael Park <mc...@gmail.com>.
> On April 23, 2015, 5:35 p.m., Jie Yu wrote:
> > src/tests/resources_tests.cpp, line 835
> > <https://reviews.apache.org/r/32140/diff/6/?file=939887#file939887line835>
> >
> > Move this to src/tests/mesos.hpp close to createPersistentVolume?
I believe you're referring to `createDiskResource`. I kept `createReservedResource` close to the tests that use it.
```cpp
static Resource createReservedResource(...) { ... }
// Tests that use createReservedResource...
static Resource createDiskResource(...) { ... }
// Tests that use createDiskResource...
```
Would it be better to pull up the `createDiskResource` to be:
```cpp
static Resource createReservedResource(...) { ... }
static Resource createDiskResource(...) { ... }
// Tests that use createReservedResource...
// Tests that use createDiskResource...
```
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review81342
-----------------------------------------------------------
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
>
>
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review81342
-----------------------------------------------------------
src/common/resources.cpp
<https://reviews.apache.org/r/32140/#comment131681>
The DiskInfo should match at this point. So
```
if (left.has_disk() && left.disk().has_persistence())
```
should be sufficient.
src/common/resources.cpp
<https://reviews.apache.org/r/32140/#comment131682>
See my comments in https://reviews.apache.org/r/32150/
Can we move this to master validation? Thoughts?
src/tests/resources_tests.cpp
<https://reviews.apache.org/r/32140/#comment131688>
Move this to src/tests/mesos.hpp close to createPersistentVolume?
src/tests/resources_tests.cpp
<https://reviews.apache.org/r/32140/#comment131689>
Nits. Just to be consistent, please use the same style:
```
EXPECT_NONE(Resources::validate(createReservedResource(
"cpus", ..., ..., ...));
```
src/tests/resources_tests.cpp
<https://reviews.apache.org/r/32140/#comment131684>
Please move '{' to the next line.
src/tests/resources_tests.cpp
<https://reviews.apache.org/r/32140/#comment131685>
Ditto.
src/tests/resources_tests.cpp
<https://reviews.apache.org/r/32140/#comment131690>
Mind splitting into two tests?
AdditionStaticallyReserved
AdditionDynamicallyReserved
src/tests/resources_tests.cpp
<https://reviews.apache.org/r/32140/#comment131686>
Ditto.
- Jie Yu
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
>
>
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review81378
-----------------------------------------------------------
Ship it!
Discussed with Mpark offline. We agreed that rule for Resources::validate is that it should only perform necessary validation to make sure all methods in Resources are well hahaved, and the validation around * and reservation info is necessary for 'reserved/unreserved' to work properly. Thus dropping the issue around validation.
- Jie Yu
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
>
>
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
Addressed some of the comments from AlexR, Tim and Jie.
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 (updated)
-----
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
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
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.
Summary (updated)
-----------------
Enabled 'Resources' to handle 'Resource::ReservationInfo'.
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
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Jie Yu <yu...@gmail.com>.
> 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.
>
> Michael Park wrote:
> 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?
See my first comment in https://reviews.apache.org/r/32150/
If we move the validation to master, then this function becomes weired.
- Jie
-----------------------------------------------------------
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
>
>
Re: Review Request 32140: Enable 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> 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?
Can we have a resource with `role == "*"` and reservations set?
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review79410
-----------------------------------------------------------
On April 7, 2015, 9:56 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32140/
> -----------------------------------------------------------
>
> (Updated April 7, 2015, 9:56 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
>
>
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Michael Park <mc...@gmail.com>.
> 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
>
>
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Michael Park <mc...@gmail.com>.
> 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.
>
> Michael Park wrote:
> 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?
>
> Jie Yu wrote:
> See my first comment in https://reviews.apache.org/r/32150/
>
> If we move the validation to master, then this function becomes weired.
Refer to https://reviews.apache.org/r/32140/#comment131682
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review79410
-----------------------------------------------------------
On April 23, 2015, 8:25 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32140/
> -----------------------------------------------------------
>
> (Updated April 23, 2015, 8:25 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
>
>
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> 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?
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.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review79410
-----------------------------------------------------------
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
>
>
Re: Review Request 32140: Enable 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review79410
-----------------------------------------------------------
src/common/resources.cpp
<https://reviews.apache.org/r/32140/#comment128732>
I probably would split the logic like below so that it's more modular. Also, I think the current semantics for DiskInfo matching is a little weired (my fault) and I don't think we have a use case for now. So let's make it consistent with the ReservationInfo check (and remove the NOTE). You probably need to adjust some ResourcesTest.
```
if (left.name() != right.name() ||
left.type() != right.type() ||
left.role() != right.role()) {
return false;
}
// Check if ReservationInfo matches.
if (left.has_reservation() != right.has_reservation()) {
return false;
}
if (left.has_reservation() && left.reservation() != right.reservation())) {
return false;
}
// Check if DiskInfo matches.
if (left.has_disk() != right.has_disk()) {
return false;
}
if (left.has_disk() && left.disk() != right.disk()) {
return false;
}
```
src/common/resources.cpp
<https://reviews.apache.org/r/32140/#comment128738>
Ditto on modularity.
```
if (left.name() != right.name() ||
left.type() != right.type() ||
left.role() != right.role()) {
return false;
}
// Check ReservationInfo.
if (left.has_reservation() != right.has_reservation()) {
return false;
}
if (left.has_reservation() && left.reservation() != right.reservation()) {
return false;
}
// Check DiskInfo.
if (left.has_disk() != right.has_disk()) {
return false;
}
if (left.has_disk() && left.disk() != right.disk()) {
return false;
}
// TODO(jieyu): Even if two ....
if (left.has_disk() && left.disk().has_persistence()) {
return false;
}
return true;
```
src/common/resources.cpp
<https://reviews.apache.org/r/32140/#comment128753>
Ditto on modularity.
```
if (left.name() != right.name() ||
left.type() != right.type() ||
left.role() != right.role()) {
return false;
}
// Check ReservationInfo.
if (left.has_reservation() != right.has_reservation()) {
return false;
}
if (left.has_reservation() && left.reservation() != right.reservation()) {
return false;
}
// Check DiskInfo.
if (left.has_disk() != right.has_disk()) {
return false;
}
if (left.has_disk() && left.disk() != right.disk()) {
return false;
}
if (left.has_disk() && left.disk().has_persistence() && left != right) {
return false;
}
return true;
```
src/common/resources.cpp
<https://reviews.apache.org/r/32140/#comment128760>
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?
- Jie Yu
On April 7, 2015, 9:56 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32140/
> -----------------------------------------------------------
>
> (Updated April 7, 2015, 9:56 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
>
>
Re: Review Request 32140: Enable 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/
-----------------------------------------------------------
(Updated April 7, 2015, 9:56 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 (updated)
-----
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
Re: Review Request 32140: Enabled 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Michael Park <mc...@gmail.com>.
> 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
>
>
Re: Review Request 32140: Enable 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review77752
-----------------------------------------------------------
include/mesos/resources.hpp
<https://reviews.apache.org/r/32140/#comment125948>
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.
src/common/resources.cpp
<https://reviews.apache.org/r/32140/#comment125949>
Why is this check necessary if we're already checking it above?
src/common/resources.cpp
<https://reviews.apache.org/r/32140/#comment125950>
same here
src/common/resources.cpp
<https://reviews.apache.org/r/32140/#comment125951>
same here
src/common/resources.cpp
<https://reviews.apache.org/r/32140/#comment125953>
Why check framework Id? Shouldn't the first 2 checks be enough?
src/common/resources.cpp
<https://reviews.apache.org/r/32140/#comment125954>
As we already checked for * don't have reservation, seems like we should just check for has_reservation then?
- Timothy Chen
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
>
>
Re: Review Request 32140: Enable 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
"unreserved" resource requires that (role, reservation) pair to be `("*", None)`.
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 (updated)
-----
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
Re: Review Request 32140: Enable 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
Frameworks with role "\*" should be able to reserve resources for itself.
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 (updated)
-----
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
Re: Review Request 32140: Enable 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/
-----------------------------------------------------------
(Updated March 18, 2015, 5:25 p.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
Changes
-------
Added tests and fixed a bug.
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 (updated)
-----
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
Re: Review Request 32140: Enable 'Resources' to handle
'Resource::ReservationInfo'.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/
-----------------------------------------------------------
(Updated March 17, 2015, 4:06 a.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
Diff: https://reviews.apache.org/r/32140/diff/
Testing
-------
make check
Thanks,
Michael Park