Re: Review Request 35715: Added revocable resource state validation.

2015-07-16 Thread Niklas Nielsen


 On June 21, 2015, 11:47 a.m., Vinod Kone wrote:
  src/common/resources.cpp, lines 479-487
  https://reviews.apache.org/r/35715/diff/1/?file=989223#file989223line479
 
  These checks are done in master's validation.cpp
 
 Michael Park wrote:
 Ah sorry, I missed that.
 
 This reminded me of the discussion Jie and I had for 
 [r32140](https://reviews.apache.org/r/32140/) regarding where validations 
 should live. I think this validation belongs here rather than in master 
 validation.
 What we concluded from the discussion was that `Resources::validate` 
 should perform necessary validation to satisfy the invariant of the 
 `Resource` object.
 This enables methods that operate on `Resource` (e.g. 
 `Resources::isRevocable`) to assume its validity.
 
 My notes:
  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.
 
 Jie's note:
  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.
 
 Michael Park wrote:
 I found Jie's comment regarding this: 
 https://reviews.apache.org/r/33865/#comment133597
 
 @Jie: My thought here was that these checks are necessary to make 
 `isRevocable` well-defined. The same way the check for `* resource cannot 
 be dynamically reserved` is necessary to make `isDynamicallyReserved` and 
 others well-defined?
 
 Jie Yu wrote:
 @Mpark,
 
 I think the following check is in Resources::validate because otherwise 
 isReserved will break (e.g., role = `*` and reservation is not set, 
 isReserved(resource, `*`) will return true).
 
 ```
 if (resource.role() == *  resource.has_reservation()) {
   return Error(
   Invalid reservation: role \*\ cannot be dynamically reserved);
 }
 ```
 
 Michael Park wrote:
 @Jie,
  e.g., role = * and reservation is not set, isReserved(resource, *) will 
 return true
 
 If you meant `role = * and reservation _is_ set`, then yes.
 
 I'm saying that exact reasoning is also why these checks should be in 
 `Resources::validate`, because otherwise `isRevocable` will break.
 e.g. `reservation is set and revocable is set`, `isRevocable` will return 
 true.
 
 Niklas Nielsen wrote:
 Hey guys - did you reach a conclusion?

MPark; how can we get closure on this?


- Niklas


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


On June 21, 2015, noon, Michael Park wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/35715/
 ---
 
 (Updated June 21, 2015, noon)
 
 
 Review request for mesos, Jie Yu, Niklas Nielsen, and Vinod Kone.
 
 
 Repository: mesos
 
 
 Description
 ---
 
 In `mesos.proto`, it specifies the expected state of revocable resource:
 
 ```
 // ... Note that if this is set, 'disk' or 'reservation' cannot be set.
 optional RevocableInfo revocable = 9;
 ```
   
 This expectation should be validated in `Resources::validate(const Resource 
 resoure)`
 
 
 Diffs
 -
 
   src/common/resources.cpp eb5476a0365fe65f474afd0ab7a52ad7f1e04521 
   src/tests/resources_tests.cpp 9f96b14a6a4ce416d044934dd7ab4d28e4bc7332 
 
 Diff: https://reviews.apache.org/r/35715/diff/
 
 
 Testing
 ---
 
 Added `RevocableResourceTest.Validation` + `make check`
 
 
 Thanks,
 
 Michael Park
 




Re: Review Request 35715: Added revocable resource state validation.

2015-07-16 Thread Michael Park


 On June 21, 2015, 6:47 p.m., Vinod Kone wrote:
  src/common/resources.cpp, lines 479-487
  https://reviews.apache.org/r/35715/diff/1/?file=989223#file989223line479
 
  These checks are done in master's validation.cpp
 
 Michael Park wrote:
 Ah sorry, I missed that.
 
 This reminded me of the discussion Jie and I had for 
 [r32140](https://reviews.apache.org/r/32140/) regarding where validations 
 should live. I think this validation belongs here rather than in master 
 validation.
 What we concluded from the discussion was that `Resources::validate` 
 should perform necessary validation to satisfy the invariant of the 
 `Resource` object.
 This enables methods that operate on `Resource` (e.g. 
 `Resources::isRevocable`) to assume its validity.
 
 My notes:
  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.
 
 Jie's note:
  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.
 
 Michael Park wrote:
 I found Jie's comment regarding this: 
 https://reviews.apache.org/r/33865/#comment133597
 
 @Jie: My thought here was that these checks are necessary to make 
 `isRevocable` well-defined. The same way the check for `* resource cannot 
 be dynamically reserved` is necessary to make `isDynamicallyReserved` and 
 others well-defined?
 
 Jie Yu wrote:
 @Mpark,
 
 I think the following check is in Resources::validate because otherwise 
 isReserved will break (e.g., role = `*` and reservation is not set, 
 isReserved(resource, `*`) will return true).
 
 ```
 if (resource.role() == *  resource.has_reservation()) {
   return Error(
   Invalid reservation: role \*\ cannot be dynamically reserved);
 }
 ```
 
 Michael Park wrote:
 @Jie,
  e.g., role = * and reservation is not set, isReserved(resource, *) will 
 return true
 
 If you meant `role = * and reservation _is_ set`, then yes.
 
 I'm saying that exact reasoning is also why these checks should be in 
 `Resources::validate`, because otherwise `isRevocable` will break.
 e.g. `reservation is set and revocable is set`, `isRevocable` will return 
 true.
 
 Niklas Nielsen wrote:
 Hey guys - did you reach a conclusion?
 
 Niklas Nielsen wrote:
 MPark; how can we get closure on this?

I asked Jie to look at it a while ago but I think he's quite busy. I'll discard 
it for now.


- Michael


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


On June 21, 2015, 7 p.m., Michael Park wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/35715/
 ---
 
 (Updated June 21, 2015, 7 p.m.)
 
 
 Review request for mesos, Jie Yu, Niklas Nielsen, and Vinod Kone.
 
 
 Repository: mesos
 
 
 Description
 ---
 
 In `mesos.proto`, it specifies the expected state of revocable resource:
 
 ```
 // ... Note that if this is set, 'disk' or 'reservation' cannot be set.
 optional RevocableInfo revocable = 9;
 ```
   
 This expectation should be validated in `Resources::validate(const Resource 
 resoure)`
 
 
 Diffs
 -
 
   src/common/resources.cpp eb5476a0365fe65f474afd0ab7a52ad7f1e04521 
   src/tests/resources_tests.cpp 9f96b14a6a4ce416d044934dd7ab4d28e4bc7332 
 
 Diff: https://reviews.apache.org/r/35715/diff/
 
 
 Testing
 ---
 
 Added `RevocableResourceTest.Validation` + `make check`
 
 
 Thanks,
 
 Michael Park
 




Re: Review Request 35715: Added revocable resource state validation.

2015-06-22 Thread Jie Yu


 On June 21, 2015, 6:47 p.m., Vinod Kone wrote:
  src/common/resources.cpp, lines 479-487
  https://reviews.apache.org/r/35715/diff/1/?file=989223#file989223line479
 
  These checks are done in master's validation.cpp
 
 Michael Park wrote:
 Ah sorry, I missed that.
 
 This reminded me of the discussion Jie and I had for 
 [r32140](https://reviews.apache.org/r/32140/) regarding where validations 
 should live. I think this validation belongs here rather than in master 
 validation.
 What we concluded from the discussion was that `Resources::validate` 
 should perform necessary validation to satisfy the invariant of the 
 `Resource` object.
 This enables methods that operate on `Resource` (e.g. 
 `Resources::isRevocable`) to assume its validity.
 
 My notes:
  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.
 
 Jie's note:
  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.
 
 Michael Park wrote:
 I found Jie's comment regarding this: 
 https://reviews.apache.org/r/33865/#comment133597
 
 @Jie: My thought here was that these checks are necessary to make 
 `isRevocable` well-defined. The same way the check for `* resource cannot 
 be dynamically reserved` is necessary to make `isDynamicallyReserved` and 
 others well-defined?

@Mpark,

I think the following check is in Resources::validate because otherwise 
isReserved will break (e.g., role = `*` and reservation is not set, 
isReserved(resource, `*`) will return true).

```
if (resource.role() == *  resource.has_reservation()) {
  return Error(
  Invalid reservation: role \*\ cannot be dynamically reserved);
}
```


- Jie


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


On June 21, 2015, 7 p.m., Michael Park wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/35715/
 ---
 
 (Updated June 21, 2015, 7 p.m.)
 
 
 Review request for mesos, Jie Yu, Niklas Nielsen, and Vinod Kone.
 
 
 Repository: mesos
 
 
 Description
 ---
 
 In `mesos.proto`, it specifies the expected state of revocable resource:
 
 ```
 // ... Note that if this is set, 'disk' or 'reservation' cannot be set.
 optional RevocableInfo revocable = 9;
 ```
   
 This expectation should be validated in `Resources::validate(const Resource 
 resoure)`
 
 
 Diffs
 -
 
   src/common/resources.cpp eb5476a0365fe65f474afd0ab7a52ad7f1e04521 
   src/tests/resources_tests.cpp 9f96b14a6a4ce416d044934dd7ab4d28e4bc7332 
 
 Diff: https://reviews.apache.org/r/35715/diff/
 
 
 Testing
 ---
 
 Added `RevocableResourceTest.Validation` + `make check`
 
 
 Thanks,
 
 Michael Park