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/04/08 19:04:50 UTC
Re: Review Request 32150: Enable the master to handle reservation
operations.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/
-----------------------------------------------------------
(Updated April 8, 2015, 5:04 p.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
Bugs: MESOS-2139
https://issues.apache.org/jira/browse/MESOS-2139
Repository: mesos
Description
-------
Handled reservation operations in `Master::_accept`.
Added `validate` functions in `src/master/validation.{hpp,cpp}`.
Diffs (updated)
-----
src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b
src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
src/master/validation.cpp 2f2e4ea8ea123c5a0d01446cdec8b308ea60932e
src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
Diff: https://reviews.apache.org/r/32150/diff/
Testing
-------
make check
Thanks,
Michael Park
Re: Review Request 32150: Enabled the master to handle reservation
operations.
Posted by Michael Park <mc...@gmail.com>.
> On April 22, 2015, 12:38 p.m., Alexander Rukletsov wrote:
> > src/master/validation.cpp, line 555
> > <https://reviews.apache.org/r/32150/diff/5/?file=920955#file920955line555>
> >
> > Let's leave a comment here that `resource::validate` not only checks for integrity of the `resources` instance, but also for inconsistent state related to dynamic reservations. Since `resource::validate` doesn't have any comment it's unclear here that it does a related but non-obvious check for "*" role.
Do you find that this is still related or helpful since our discussion [here](https://reviews.apache.org/r/32140/#comment131682) along with the listed valid states of the `Resource` object?
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/#review81139
-----------------------------------------------------------
On April 27, 2015, 10:10 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32150/
> -----------------------------------------------------------
>
> (Updated April 27, 2015, 10:10 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2139
> https://issues.apache.org/jira/browse/MESOS-2139
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled reservation operations in `Master::_accept`.
>
> Added `validate` functions in `src/master/validation.{hpp,cpp}`.
>
>
> Diffs
> -----
>
> src/master/master.cpp d42a6f321c88ec5d0418264bdda39d083ff54a7e
> src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
> src/master/validation.cpp dc25995bf57397d42fcde458414f0402d19bf792
> src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
>
> Diff: https://reviews.apache.org/r/32150/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 32150: Enabled the master to handle reservation
operations.
Posted by Michael Park <mc...@gmail.com>.
> On April 22, 2015, 12:38 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, lines 2458-2459
> > <https://reviews.apache.org/r/32150/diff/5/?file=920953#file920953line2458>
> >
> > These two fields are optional, `principal` doesn't have a default. Do we need to check it? Can a framework without a principal reserve resources (the answer is no, I suppose, because in the current desgin a principal register resources, not a framework)?
> >
> > DO you mind adding tests for these cases?
Thanks for this! Fixed and added a test `FrameworkMissingPrincipal`.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/#review81139
-----------------------------------------------------------
On April 28, 2015, 10:21 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32150/
> -----------------------------------------------------------
>
> (Updated April 28, 2015, 10:21 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2139
> https://issues.apache.org/jira/browse/MESOS-2139
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled reservation operations in `Master::_accept`.
>
> Added `validate` functions in `src/master/validation.{hpp,cpp}`.
>
>
> Diffs
> -----
>
> src/master/master.cpp d42a6f321c88ec5d0418264bdda39d083ff54a7e
> src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
> src/master/validation.cpp dc25995bf57397d42fcde458414f0402d19bf792
> src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
>
> Diff: https://reviews.apache.org/r/32150/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 32150: Enabled the master to handle reservation
operations.
Posted by Michael Park <mc...@gmail.com>.
> On April 22, 2015, 12:38 p.m., Alexander Rukletsov wrote:
> > src/tests/master_validation_tests.cpp, line 328
> > <https://reviews.apache.org/r/32150/diff/5/?file=920956#file920956line328>
> >
> > Don't you get an unused variable warning?
Hm, looks like I do. Not sure what happpened there. Sorry about that.
> On April 22, 2015, 12:38 p.m., Alexander Rukletsov wrote:
> > src/tests/master_validation_tests.cpp, line 176
> > <https://reviews.apache.org/r/32150/diff/5/?file=920956#file920956line176>
> >
> > const?
These variables have been removed as per Jie request.
> On April 22, 2015, 12:38 p.m., Alexander Rukletsov wrote:
> > src/tests/master_validation_tests.cpp, line 249
> > <https://reviews.apache.org/r/32150/diff/5/?file=920956#file920956line249>
> >
> > const? here and below.
These variables have been removed as per Jie's request.
> On April 22, 2015, 12:38 p.m., Alexander Rukletsov wrote:
> > src/tests/master_validation_tests.cpp, line 314
> > <https://reviews.apache.org/r/32150/diff/5/?file=920956#file920956line314>
> >
> > You don't use `principal` in unreserve, is this comment valid then? As I have already mentioned, I would rather check for principal match though.
As you point out, the 2 tests were actually functionally equivalent so I've collapsed them into one. See my comment above as for why we don't check the `principal` match.
> On April 22, 2015, 12:38 p.m., Alexander Rukletsov wrote:
> > src/master/validation.cpp, lines 591-592
> > <https://reviews.apache.org/r/32150/diff/5/?file=920955#file920955line591>
> >
> > Shall we check framework and request principals are the same?
No, at least not in the current specification. The `principal` indicates the `principal` of the operator or framework that reserved the resources, which means that we need to make sure that `Resource.Reservation.principal` and `FrameworkInfo.principal` matches for the `Reserve` operation. On `Unreserve`, we use the operator or framework's `principal` and the `principal` set on the `Resource` object in conjunction with the "unreserve" ACL to determine whether the unreserve operation is authorized. In the absense of an ACL, we allow anyone to unreserve any resources. This is a matter of authorization however, so we wouldn't check for the `prinicpal` matching in `validate` even with ACLs.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/#review81139
-----------------------------------------------------------
On April 27, 2015, 10:10 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32150/
> -----------------------------------------------------------
>
> (Updated April 27, 2015, 10:10 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2139
> https://issues.apache.org/jira/browse/MESOS-2139
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled reservation operations in `Master::_accept`.
>
> Added `validate` functions in `src/master/validation.{hpp,cpp}`.
>
>
> Diffs
> -----
>
> src/master/master.cpp d42a6f321c88ec5d0418264bdda39d083ff54a7e
> src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
> src/master/validation.cpp dc25995bf57397d42fcde458414f0402d19bf792
> src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
>
> Diff: https://reviews.apache.org/r/32150/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 32150: Enabled the master to handle reservation
operations.
Posted by Michael Park <mc...@gmail.com>.
> On April 22, 2015, 12:38 p.m., Alexander Rukletsov wrote:
> > src/tests/master_validation_tests.cpp, line 247
> > <https://reviews.apache.org/r/32150/diff/5/?file=920956#file920956line247>
> >
> > As I have mentioned earlier, how about a test where we `FrameworkInfo` doesn't have a principal?
Added a test called `FrameworkMissingPrincipal`.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/#review81139
-----------------------------------------------------------
On April 28, 2015, 10:21 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32150/
> -----------------------------------------------------------
>
> (Updated April 28, 2015, 10:21 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2139
> https://issues.apache.org/jira/browse/MESOS-2139
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled reservation operations in `Master::_accept`.
>
> Added `validate` functions in `src/master/validation.{hpp,cpp}`.
>
>
> Diffs
> -----
>
> src/master/master.cpp d42a6f321c88ec5d0418264bdda39d083ff54a7e
> src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
> src/master/validation.cpp dc25995bf57397d42fcde458414f0402d19bf792
> src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
>
> Diff: https://reviews.apache.org/r/32150/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 32150: Enabled the master to handle reservation
operations.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On April 22, 2015, 12:38 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, lines 2458-2459
> > <https://reviews.apache.org/r/32150/diff/5/?file=920953#file920953line2458>
> >
> > These two fields are optional, `principal` doesn't have a default. Do we need to check it? Can a framework without a principal reserve resources (the answer is no, I suppose, because in the current desgin a principal register resources, not a framework)?
> >
> > DO you mind adding tests for these cases?
>
> Michael Park wrote:
> Thanks for this! Fixed and added a test `FrameworkMissingPrincipal`.
>
> Michael Park wrote:
> Additionally, I've made changes to also reject unreservation requests from frameworks without a `principal`. This makes it so that even without an ACL, frameworks without a `principal` are completely out of the picture for dynamic reservations.
That sounds right.
> On April 22, 2015, 12:38 p.m., Alexander Rukletsov wrote:
> > src/master/validation.cpp, line 555
> > <https://reviews.apache.org/r/32150/diff/5/?file=920955#file920955line555>
> >
> > Let's leave a comment here that `resource::validate` not only checks for integrity of the `resources` instance, but also for inconsistent state related to dynamic reservations. Since `resource::validate` doesn't have any comment it's unclear here that it does a related but non-obvious check for "*" role.
>
> Michael Park wrote:
> Do you find that this is still related or helpful since our discussion [here](https://reviews.apache.org/r/32140/#comment131682) along with the listed valid states of the `Resource` object?
I think it won't hurt leaving a pointer to the next guy who will be reading this code : ).
> On April 22, 2015, 12:38 p.m., Alexander Rukletsov wrote:
> > src/master/validation.cpp, lines 591-592
> > <https://reviews.apache.org/r/32150/diff/5/?file=920955#file920955line591>
> >
> > Shall we check framework and request principals are the same?
>
> Michael Park wrote:
> No, at least not in the current specification. The `principal` indicates the `principal` of the operator or framework that reserved the resources, which means that we need to make sure that `Resource.Reservation.principal` and `FrameworkInfo.principal` matches for the `Reserve` operation. On `Unreserve`, we use the operator or framework's `principal` and the `principal` set on the `Resource` object in conjunction with the "unreserve" ACL to determine whether the unreserve operation is authorized. In the absense of an ACL, we allow anyone to unreserve any resources. This is a matter of authorization however, so we wouldn't check for the `prinicpal` matching in `validate` even with ACLs.
> In the absense of an ACL, we allow anyone to unreserve any resources.
This is counter-intuitive for me, can we write this as a comment? Also, feel free to drop once done.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/#review81139
-----------------------------------------------------------
On April 28, 2015, 10:43 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32150/
> -----------------------------------------------------------
>
> (Updated April 28, 2015, 10:43 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2139
> https://issues.apache.org/jira/browse/MESOS-2139
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled reservation operations in `Master::_accept`.
>
> Added `validate` functions in `src/master/validation.{hpp,cpp}`.
>
>
> Diffs
> -----
>
> src/master/master.cpp d42a6f321c88ec5d0418264bdda39d083ff54a7e
> src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
> src/master/validation.cpp dc25995bf57397d42fcde458414f0402d19bf792
> src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
>
> Diff: https://reviews.apache.org/r/32150/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 32150: Enabled the master to handle reservation
operations.
Posted by Michael Park <mc...@gmail.com>.
> On April 22, 2015, 12:38 p.m., Alexander Rukletsov wrote:
> > src/master/validation.cpp, line 555
> > <https://reviews.apache.org/r/32150/diff/5/?file=920955#file920955line555>
> >
> > Let's leave a comment here that `resource::validate` not only checks for integrity of the `resources` instance, but also for inconsistent state related to dynamic reservations. Since `resource::validate` doesn't have any comment it's unclear here that it does a related but non-obvious check for "*" role.
>
> Michael Park wrote:
> Do you find that this is still related or helpful since our discussion [here](https://reviews.apache.org/r/32140/#comment131682) along with the listed valid states of the `Resource` object?
>
> Alexander Rukletsov wrote:
> I think it won't hurt leaving a pointer to the next guy who will be reading this code : ).
I've updated the comment for `Resource::validate` like so:
```
A Resource object is valid if it has a name, a valid type,
- // i.e. scalar, range, or set, and has the appropriate value set.
+ // i.e. scalar, range, or set, has the appropriate value set, and
+ // a valid (role, reservation) pair for dynamic reservation.
```
> On April 22, 2015, 12:38 p.m., Alexander Rukletsov wrote:
> > src/master/validation.cpp, lines 591-592
> > <https://reviews.apache.org/r/32150/diff/5/?file=920955#file920955line591>
> >
> > Shall we check framework and request principals are the same?
>
> Michael Park wrote:
> No, at least not in the current specification. The `principal` indicates the `principal` of the operator or framework that reserved the resources, which means that we need to make sure that `Resource.Reservation.principal` and `FrameworkInfo.principal` matches for the `Reserve` operation. On `Unreserve`, we use the operator or framework's `principal` and the `principal` set on the `Resource` object in conjunction with the "unreserve" ACL to determine whether the unreserve operation is authorized. In the absense of an ACL, we allow anyone to unreserve any resources. This is a matter of authorization however, so we wouldn't check for the `prinicpal` matching in `validate` even with ACLs.
>
> Alexander Rukletsov wrote:
> > In the absense of an ACL, we allow anyone to unreserve any resources.
>
> This is counter-intuitive for me, can we write this as a comment? Also, feel free to drop once done.
I've left the following comment in the `validate` function for `Unreserve`:
```
// NOTE: We don't check that 'FrameworkInfo.principal' matches
// 'Resource.ReservationInfo.principal' here because the authorization
// depends on the "unreserve" ACL which specifies which 'principal' can
// unreserve which 'principal's resources. In the absense of an ACL, we allow
// any 'principal' to unreserve any other 'principal's resources.
```
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/#review81139
-----------------------------------------------------------
On May 2, 2015, 12:27 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32150/
> -----------------------------------------------------------
>
> (Updated May 2, 2015, 12:27 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2139
> https://issues.apache.org/jira/browse/MESOS-2139
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled reservation operations in `Master::_accept`.
>
> Added `validate` functions in `src/master/validation.{hpp,cpp}`.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 56affd45e1e71e96ff5778b43271f81b9b9a9e4d
> src/master/master.cpp d42a6f321c88ec5d0418264bdda39d083ff54a7e
> src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
> src/master/validation.cpp dc25995bf57397d42fcde458414f0402d19bf792
> src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
>
> Diff: https://reviews.apache.org/r/32150/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 32150: Enabled the master to handle reservation
operations.
Posted by Michael Park <mc...@gmail.com>.
> On April 22, 2015, 12:38 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, lines 2458-2459
> > <https://reviews.apache.org/r/32150/diff/5/?file=920953#file920953line2458>
> >
> > These two fields are optional, `principal` doesn't have a default. Do we need to check it? Can a framework without a principal reserve resources (the answer is no, I suppose, because in the current desgin a principal register resources, not a framework)?
> >
> > DO you mind adding tests for these cases?
>
> Michael Park wrote:
> Thanks for this! Fixed and added a test `FrameworkMissingPrincipal`.
Additionally, I've made changes to also reject unreservation requests from frameworks without a `principal`. This makes it so that even without an ACL, frameworks without a `principal` are completely out of the picture for dynamic reservations.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/#review81139
-----------------------------------------------------------
On April 28, 2015, 10:43 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32150/
> -----------------------------------------------------------
>
> (Updated April 28, 2015, 10:43 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2139
> https://issues.apache.org/jira/browse/MESOS-2139
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled reservation operations in `Master::_accept`.
>
> Added `validate` functions in `src/master/validation.{hpp,cpp}`.
>
>
> Diffs
> -----
>
> src/master/master.cpp d42a6f321c88ec5d0418264bdda39d083ff54a7e
> src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
> src/master/validation.cpp dc25995bf57397d42fcde458414f0402d19bf792
> src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
>
> Diff: https://reviews.apache.org/r/32150/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 32150: Enabled the master to handle reservation
operations.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/#review81139
-----------------------------------------------------------
Looks good to me, MPark.
Several high-level questions (some of them are duplicated in the issues):
* Why don't we want to check that request `principal` matches `FrameworkInfo.principal` in unreserve operation?
* Can we dynamically unreserve statically reserved resources? Could you please add a test for this?
src/master/master.cpp
<https://reviews.apache.org/r/32150/#comment131401>
These two fields are optional, `principal` doesn't have a default. Do we need to check it? Can a framework without a principal reserve resources (the answer is no, I suppose, because in the current desgin a principal register resources, not a framework)?
DO you mind adding tests for these cases?
src/master/validation.cpp
<https://reviews.apache.org/r/32150/#comment131404>
Let's leave a comment here that `resource::validate` not only checks for integrity of the `resources` instance, but also for inconsistent state related to dynamic reservations. Since `resource::validate` doesn't have any comment it's unclear here that it does a related but non-obvious check for "*" role.
src/master/validation.cpp
<https://reviews.apache.org/r/32150/#comment131407>
Shall we check framework and request principals are the same?
src/tests/master_validation_tests.cpp
<https://reviews.apache.org/r/32150/#comment131410>
const?
src/tests/master_validation_tests.cpp
<https://reviews.apache.org/r/32150/#comment131411>
As I have mentioned earlier, how about a test where we `FrameworkInfo` doesn't have a principal?
src/tests/master_validation_tests.cpp
<https://reviews.apache.org/r/32150/#comment131412>
const? here and below.
src/tests/master_validation_tests.cpp
<https://reviews.apache.org/r/32150/#comment131413>
You don't use `principal` in unreserve, is this comment valid then? As I have already mentioned, I would rather check for principal match though.
src/tests/master_validation_tests.cpp
<https://reviews.apache.org/r/32150/#comment131414>
Don't you get an unused variable warning?
- Alexander Rukletsov
On April 15, 2015, 3:55 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32150/
> -----------------------------------------------------------
>
> (Updated April 15, 2015, 3:55 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2139
> https://issues.apache.org/jira/browse/MESOS-2139
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled reservation operations in `Master::_accept`.
>
> Added `validate` functions in `src/master/validation.{hpp,cpp}`.
>
>
> Diffs
> -----
>
> src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b
> src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
> src/master/validation.cpp 2f2e4ea8ea123c5a0d01446cdec8b308ea60932e
> src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
>
> Diff: https://reviews.apache.org/r/32150/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 32150: Enabled the master to handle reservation
operations.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/
-----------------------------------------------------------
(Updated May 2, 2015, 12:27 a.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
Changes
-------
Addressed comments.
Bugs: MESOS-2139
https://issues.apache.org/jira/browse/MESOS-2139
Repository: mesos
Description
-------
Handled reservation operations in `Master::_accept`.
Added `validate` functions in `src/master/validation.{hpp,cpp}`.
Diffs (updated)
-----
include/mesos/resources.hpp 56affd45e1e71e96ff5778b43271f81b9b9a9e4d
src/master/master.cpp d42a6f321c88ec5d0418264bdda39d083ff54a7e
src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
src/master/validation.cpp dc25995bf57397d42fcde458414f0402d19bf792
src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
Diff: https://reviews.apache.org/r/32150/diff/
Testing
-------
make check
Thanks,
Michael Park
Re: Review Request 32150: Enabled the master to handle reservation
operations.
Posted by Michael Park <mc...@gmail.com>.
> On April 29, 2015, 10:15 p.m., Jie Yu wrote:
> > src/tests/master_validation_tests.cpp, line 286
> > <https://reviews.apache.org/r/32150/diff/8/?file=944525#file944525line286>
> >
> > One extra blank line here please.
Added.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/#review82045
-----------------------------------------------------------
On May 2, 2015, 12:27 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32150/
> -----------------------------------------------------------
>
> (Updated May 2, 2015, 12:27 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2139
> https://issues.apache.org/jira/browse/MESOS-2139
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled reservation operations in `Master::_accept`.
>
> Added `validate` functions in `src/master/validation.{hpp,cpp}`.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 56affd45e1e71e96ff5778b43271f81b9b9a9e4d
> src/master/master.cpp d42a6f321c88ec5d0418264bdda39d083ff54a7e
> src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
> src/master/validation.cpp dc25995bf57397d42fcde458414f0402d19bf792
> src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
>
> Diff: https://reviews.apache.org/r/32150/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 32150: Enabled the master to handle reservation
operations.
Posted by Michael Park <mc...@gmail.com>.
> On April 29, 2015, 10:15 p.m., Jie Yu wrote:
> > src/master/master.cpp, lines 2487-2492
> > <https://reviews.apache.org/r/32150/diff/8/?file=944522#file944522line2487>
> >
> > Why use a tmp variable here? Could you combine with validation below:
> > ```
> > Option<Error> error = validation::operation::validate(
> > operation.reserve(),
> > framework->info.role(),
> > (framework->info.has_principal() ?
> > framework->info.principal() :
> > Option<string>::none()));
> > ```
I looked at that style and thought people would find it hard to read. But I can change it :)
> On April 29, 2015, 10:15 p.m., Jie Yu wrote:
> > src/master/validation.cpp, line 598
> > <https://reviews.apache.org/r/32150/diff/8/?file=944524#file944524line598>
> >
> > No snake_case please:)
> >
> > Also, instead of using a boolean, can you pass an Option<string> principal as you did in `validate(reserve)`?
> No snake_case please:)
Grr... the `snake_case` thing keeps getting me. I'll find a way to prevent myself from it and meanwhile be more attentive about it.
> Also, instead of using a boolean, can you pass an Option<string> principal as you did in validate(reserve)?
I considered that as well to keep "consistent", but decided that (1) we don't pass `role` so we're not really consistent anyway, and (2) we'd be passing more information than we need while hindering readability at the callsite:
```
Option<Error> error = validation::operation::validate(
operation.unreserve(),
(framework->info.has_principal() ?
framework->info.principal() :
Option<string>::none()));
```
as well as within the function: "we pass an `Option<string>` but ever only perform presence tests on it via `isSome`, `isNone`, are we missing some logic?".
I guess is that you have/had similar thoughts, but do you perhaps have strong reasons you would recommend passing `Option<string>` here?
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/#review82045
-----------------------------------------------------------
On April 28, 2015, 10:43 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32150/
> -----------------------------------------------------------
>
> (Updated April 28, 2015, 10:43 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2139
> https://issues.apache.org/jira/browse/MESOS-2139
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled reservation operations in `Master::_accept`.
>
> Added `validate` functions in `src/master/validation.{hpp,cpp}`.
>
>
> Diffs
> -----
>
> src/master/master.cpp d42a6f321c88ec5d0418264bdda39d083ff54a7e
> src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
> src/master/validation.cpp dc25995bf57397d42fcde458414f0402d19bf792
> src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
>
> Diff: https://reviews.apache.org/r/32150/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 32150: Enabled the master to handle reservation
operations.
Posted by Michael Park <mc...@gmail.com>.
> On April 29, 2015, 10:15 p.m., Jie Yu wrote:
> > src/master/validation.cpp, line 575
> > <https://reviews.apache.org/r/32150/diff/8/?file=944524#file944524line575>
> >
> > This fits into the above line?
Fixed.
> On April 29, 2015, 10:15 p.m., Jie Yu wrote:
> > src/master/validation.cpp, line 598
> > <https://reviews.apache.org/r/32150/diff/8/?file=944524#file944524line598>
> >
> > No snake_case please:)
> >
> > Also, instead of using a boolean, can you pass an Option<string> principal as you did in `validate(reserve)`?
>
> Michael Park wrote:
> > No snake_case please:)
>
> Grr... the `snake_case` thing keeps getting me. I'll find a way to prevent myself from it and meanwhile be more attentive about it.
>
> > Also, instead of using a boolean, can you pass an Option<string> principal as you did in validate(reserve)?
>
> I considered that as well to keep "consistent", but decided that (1) we don't pass `role` so we're not really consistent anyway, and (2) we'd be passing more information than we need while hindering readability at the callsite:
>
> ```
> Option<Error> error = validation::operation::validate(
> operation.unreserve(),
> (framework->info.has_principal() ?
> framework->info.principal() :
> Option<string>::none()));
> ```
>
> as well as within the function: "we pass an `Option<string>` but ever only perform presence tests on it via `isSome`, `isNone`, are we missing some logic?".
>
> I guess is that you have/had similar thoughts, but do you perhaps have strong reasons you would recommend passing `Option<string>` here?
Removed the `snake_case`, please let me know about the issue around passing a `bool` vs `Option<string>`.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/#review82045
-----------------------------------------------------------
On May 2, 2015, 12:27 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32150/
> -----------------------------------------------------------
>
> (Updated May 2, 2015, 12:27 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2139
> https://issues.apache.org/jira/browse/MESOS-2139
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled reservation operations in `Master::_accept`.
>
> Added `validate` functions in `src/master/validation.{hpp,cpp}`.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 56affd45e1e71e96ff5778b43271f81b9b9a9e4d
> src/master/master.cpp d42a6f321c88ec5d0418264bdda39d083ff54a7e
> src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
> src/master/validation.cpp dc25995bf57397d42fcde458414f0402d19bf792
> src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
>
> Diff: https://reviews.apache.org/r/32150/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 32150: Enabled the master to handle reservation
operations.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/#review82045
-----------------------------------------------------------
Ship it!
src/master/master.cpp
<https://reviews.apache.org/r/32150/#comment132680>
Why use a tmp variable here? Could you combine with validation below:
```
Option<Error> error = validation::operation::validate(
operation.reserve(),
framework->info.role(),
(framework->info.has_principal() ?
framework->info.principal() :
Option<string>::none()));
```
src/master/validation.cpp
<https://reviews.apache.org/r/32150/#comment132682>
This fits into the above line?
src/master/validation.cpp
<https://reviews.apache.org/r/32150/#comment132683>
No snake_case please:)
Also, instead of using a boolean, can you pass an Option<string> principal as you did in `validate(reserve)`?
src/tests/master_validation_tests.cpp
<https://reviews.apache.org/r/32150/#comment132685>
One extra blank line here please.
- Jie Yu
On April 28, 2015, 10:43 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32150/
> -----------------------------------------------------------
>
> (Updated April 28, 2015, 10:43 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2139
> https://issues.apache.org/jira/browse/MESOS-2139
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled reservation operations in `Master::_accept`.
>
> Added `validate` functions in `src/master/validation.{hpp,cpp}`.
>
>
> Diffs
> -----
>
> src/master/master.cpp d42a6f321c88ec5d0418264bdda39d083ff54a7e
> src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
> src/master/validation.cpp dc25995bf57397d42fcde458414f0402d19bf792
> src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
>
> Diff: https://reviews.apache.org/r/32150/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 32150: Enabled the master to handle reservation
operations.
Posted by Michael Park <mc...@gmail.com>.
> On April 29, 2015, 9:54 a.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, lines 2486-2488
> > <https://reviews.apache.org/r/32150/diff/5-8/?file=920953#file920953line2486>
> >
> > This looks a bit weird. Is it what `clang-format` proposes?
It is indeed. Do you have a preferred formatting? I couldn't really find one that I consider to be the definitive answer so I went with this.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/#review81945
-----------------------------------------------------------
On April 28, 2015, 10:43 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32150/
> -----------------------------------------------------------
>
> (Updated April 28, 2015, 10:43 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2139
> https://issues.apache.org/jira/browse/MESOS-2139
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled reservation operations in `Master::_accept`.
>
> Added `validate` functions in `src/master/validation.{hpp,cpp}`.
>
>
> Diffs
> -----
>
> src/master/master.cpp d42a6f321c88ec5d0418264bdda39d083ff54a7e
> src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
> src/master/validation.cpp dc25995bf57397d42fcde458414f0402d19bf792
> src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
>
> Diff: https://reviews.apache.org/r/32150/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 32150: Enabled the master to handle reservation
operations.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On April 29, 2015, 9:54 a.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, lines 2486-2488
> > <https://reviews.apache.org/r/32150/diff/5-8/?file=920953#file920953line2486>
> >
> > This looks a bit weird. Is it what `clang-format` proposes?
>
> Michael Park wrote:
> It is indeed. Do you have a preferred formatting? I couldn't really find one that I consider to be the definitive answer so I went with this.
If in doubt, go for consistency:
`stout/flags/flags.hpp:182`
`stout/flags/flags.hpp:247`
`libprocess/src/http.cpp:664`
I think this is simple, readable and kind of consistent with our style guide:
```
Option<string> principal = framework->info.has_principal()
? framework->info.principal()
: Option<string>::none();
```
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/#review81945
-----------------------------------------------------------
On April 28, 2015, 10:43 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32150/
> -----------------------------------------------------------
>
> (Updated April 28, 2015, 10:43 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2139
> https://issues.apache.org/jira/browse/MESOS-2139
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled reservation operations in `Master::_accept`.
>
> Added `validate` functions in `src/master/validation.{hpp,cpp}`.
>
>
> Diffs
> -----
>
> src/master/master.cpp d42a6f321c88ec5d0418264bdda39d083ff54a7e
> src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
> src/master/validation.cpp dc25995bf57397d42fcde458414f0402d19bf792
> src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
>
> Diff: https://reviews.apache.org/r/32150/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 32150: Enabled the master to handle reservation
operations.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/#review81945
-----------------------------------------------------------
Ship it!
src/master/master.cpp
<https://reviews.apache.org/r/32150/#comment132515>
This looks a bit weird. Is it what `clang-format` proposes?
- Alexander Rukletsov
On April 28, 2015, 10:43 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32150/
> -----------------------------------------------------------
>
> (Updated April 28, 2015, 10:43 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2139
> https://issues.apache.org/jira/browse/MESOS-2139
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled reservation operations in `Master::_accept`.
>
> Added `validate` functions in `src/master/validation.{hpp,cpp}`.
>
>
> Diffs
> -----
>
> src/master/master.cpp d42a6f321c88ec5d0418264bdda39d083ff54a7e
> src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
> src/master/validation.cpp dc25995bf57397d42fcde458414f0402d19bf792
> src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
>
> Diff: https://reviews.apache.org/r/32150/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 32150: Enabled the master to handle reservation
operations.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/
-----------------------------------------------------------
(Updated April 28, 2015, 10:43 p.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
Changes
-------
Reject unreservation requests from frameworks without a `principal`.
Bugs: MESOS-2139
https://issues.apache.org/jira/browse/MESOS-2139
Repository: mesos
Description
-------
Handled reservation operations in `Master::_accept`.
Added `validate` functions in `src/master/validation.{hpp,cpp}`.
Diffs (updated)
-----
src/master/master.cpp d42a6f321c88ec5d0418264bdda39d083ff54a7e
src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
src/master/validation.cpp dc25995bf57397d42fcde458414f0402d19bf792
src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
Diff: https://reviews.apache.org/r/32150/diff/
Testing
-------
make check
Thanks,
Michael Park
Re: Review Request 32150: Enabled the master to handle reservation
operations.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/
-----------------------------------------------------------
(Updated April 28, 2015, 10:21 p.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
Changes
-------
Reject reservation requests from frameworks without a `principal`.
Bugs: MESOS-2139
https://issues.apache.org/jira/browse/MESOS-2139
Repository: mesos
Description
-------
Handled reservation operations in `Master::_accept`.
Added `validate` functions in `src/master/validation.{hpp,cpp}`.
Diffs (updated)
-----
src/master/master.cpp d42a6f321c88ec5d0418264bdda39d083ff54a7e
src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
src/master/validation.cpp dc25995bf57397d42fcde458414f0402d19bf792
src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
Diff: https://reviews.apache.org/r/32150/diff/
Testing
-------
make check
Thanks,
Michael Park
Re: Review Request 32150: Enabled the master to handle reservation
operations.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/
-----------------------------------------------------------
(Updated April 27, 2015, 10:10 p.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
Changes
-------
Addressed Jie's comments.
Bugs: MESOS-2139
https://issues.apache.org/jira/browse/MESOS-2139
Repository: mesos
Description
-------
Handled reservation operations in `Master::_accept`.
Added `validate` functions in `src/master/validation.{hpp,cpp}`.
Diffs (updated)
-----
src/master/master.cpp d42a6f321c88ec5d0418264bdda39d083ff54a7e
src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
src/master/validation.cpp dc25995bf57397d42fcde458414f0402d19bf792
src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
Diff: https://reviews.apache.org/r/32150/diff/
Testing
-------
make check
Thanks,
Michael Park
Re: Review Request 32150: Enabled the master to handle reservation
operations.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/
-----------------------------------------------------------
(Updated April 15, 2015, 3:55 p.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
Summary (updated)
-----------------
Enabled the master to handle reservation operations.
Bugs: MESOS-2139
https://issues.apache.org/jira/browse/MESOS-2139
Repository: mesos
Description
-------
Handled reservation operations in `Master::_accept`.
Added `validate` functions in `src/master/validation.{hpp,cpp}`.
Diffs
-----
src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b
src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
src/master/validation.cpp 2f2e4ea8ea123c5a0d01446cdec8b308ea60932e
src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
Diff: https://reviews.apache.org/r/32150/diff/
Testing
-------
make check
Thanks,
Michael Park
Re: Review Request 32150: Enabled the master to handle reservation
operations.
Posted by Michael Park <mc...@gmail.com>.
> On April 8, 2015, 10:38 p.m., Jie Yu wrote:
> > src/master/validation.cpp, lines 581-584
> > <https://reviews.apache.org/r/32150/diff/5/?file=920955#file920955line581>
> >
> > Is this necessary, or will be captured by the resources contains check later?
It probably would be checked by `contains`, since there shouldn't be any unreserved resources with `disk` set. Having said that, I think it's worthwhile keeping this check since this error message will be more useful to the user than the error message that `contains` would produce.
> On April 8, 2015, 10:38 p.m., Jie Yu wrote:
> > src/tests/master_validation_tests.cpp, lines 178-189
> > <https://reviews.apache.org/r/32150/diff/5/?file=920956#file920956line178>
> >
> > Can you split this test into a few smaller tests (i.e., one for each {} block)? The comment in each {} block can be the test name. In that way, when a test fails, you immediately know what went wrong. What do you think?
Yeah, I agree! I'll split it in other places. In this case, the 2 tests had no functional difference so I just removed one of them.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/#review79442
-----------------------------------------------------------
On April 27, 2015, 10:10 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32150/
> -----------------------------------------------------------
>
> (Updated April 27, 2015, 10:10 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2139
> https://issues.apache.org/jira/browse/MESOS-2139
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled reservation operations in `Master::_accept`.
>
> Added `validate` functions in `src/master/validation.{hpp,cpp}`.
>
>
> Diffs
> -----
>
> src/master/master.cpp d42a6f321c88ec5d0418264bdda39d083ff54a7e
> src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
> src/master/validation.cpp dc25995bf57397d42fcde458414f0402d19bf792
> src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
>
> Diff: https://reviews.apache.org/r/32150/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 32150: Enabled the master to handle reservation
operations.
Posted by Jie Yu <yu...@gmail.com>.
> On April 8, 2015, 10:38 p.m., Jie Yu wrote:
> > src/master/validation.cpp, lines 581-584
> > <https://reviews.apache.org/r/32150/diff/5/?file=920955#file920955line581>
> >
> > Is this necessary, or will be captured by the resources contains check later?
>
> Michael Park wrote:
> It probably would be checked by `contains`, since there shouldn't be any unreserved resources with `disk` set. Having said that, I think it's worthwhile keeping this check since this error message will be more useful to the user than the error message that `contains` would produce.
SGTM. Could you please add a NOTE about the fact that this would be captured by contains check, but we would like to give better error message.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/#review79442
-----------------------------------------------------------
On April 28, 2015, 10:43 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32150/
> -----------------------------------------------------------
>
> (Updated April 28, 2015, 10:43 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2139
> https://issues.apache.org/jira/browse/MESOS-2139
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled reservation operations in `Master::_accept`.
>
> Added `validate` functions in `src/master/validation.{hpp,cpp}`.
>
>
> Diffs
> -----
>
> src/master/master.cpp d42a6f321c88ec5d0418264bdda39d083ff54a7e
> src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
> src/master/validation.cpp dc25995bf57397d42fcde458414f0402d19bf792
> src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
>
> Diff: https://reviews.apache.org/r/32150/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 32150: Enabled the master to handle reservation
operations.
Posted by Michael Park <mc...@gmail.com>.
> On April 8, 2015, 10:38 p.m., Jie Yu wrote:
> > src/master/validation.cpp, line 61
> > <https://reviews.apache.org/r/32150/diff/5/?file=920955#file920955line61>
> >
> > Do you want to add a `CHECK_NE(resource.role(), "*");` here.
> >
> > That makes me wonder should we move the check in Resources::validate about `"*"` and `ReservationInfo` here. This sounds like a more high level semantical check, while checks in Resources::validate are all quite low level (e.g., check for invalid name, type name mismatch, etc.).
> >
> > What do you think?
Synced with Jie on IRC regarding this. The summary is captured [here](https://reviews.apache.org/r/32140/#comment131682).
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/#review79442
-----------------------------------------------------------
On April 15, 2015, 3:55 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32150/
> -----------------------------------------------------------
>
> (Updated April 15, 2015, 3:55 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2139
> https://issues.apache.org/jira/browse/MESOS-2139
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled reservation operations in `Master::_accept`.
>
> Added `validate` functions in `src/master/validation.{hpp,cpp}`.
>
>
> Diffs
> -----
>
> src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b
> src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
> src/master/validation.cpp 2f2e4ea8ea123c5a0d01446cdec8b308ea60932e
> src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
>
> Diff: https://reviews.apache.org/r/32150/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 32150: Enabled the master to handle reservation
operations.
Posted by Michael Park <mc...@gmail.com>.
> On April 8, 2015, 10:38 p.m., Jie Yu wrote:
> > src/master/validation.cpp, lines 581-584
> > <https://reviews.apache.org/r/32150/diff/5/?file=920955#file920955line581>
> >
> > Is this necessary, or will be captured by the resources contains check later?
>
> Michael Park wrote:
> It probably would be checked by `contains`, since there shouldn't be any unreserved resources with `disk` set. Having said that, I think it's worthwhile keeping this check since this error message will be more useful to the user than the error message that `contains` would produce.
>
> Jie Yu wrote:
> SGTM. Could you please add a NOTE about the fact that this would be captured by contains check, but we would like to give better error message.
Added a note.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/#review79442
-----------------------------------------------------------
On May 2, 2015, 12:27 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32150/
> -----------------------------------------------------------
>
> (Updated May 2, 2015, 12:27 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2139
> https://issues.apache.org/jira/browse/MESOS-2139
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled reservation operations in `Master::_accept`.
>
> Added `validate` functions in `src/master/validation.{hpp,cpp}`.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 56affd45e1e71e96ff5778b43271f81b9b9a9e4d
> src/master/master.cpp d42a6f321c88ec5d0418264bdda39d083ff54a7e
> src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
> src/master/validation.cpp dc25995bf57397d42fcde458414f0402d19bf792
> src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
>
> Diff: https://reviews.apache.org/r/32150/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 32150: Enable the master to handle reservation
operations.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/#review79442
-----------------------------------------------------------
src/master/validation.cpp
<https://reviews.apache.org/r/32150/#comment128811>
Do you want to add a `CHECK_NE(resource.role(), "*");` here.
That makes me wonder should we move the check in Resources::validate about `"*"` and `ReservationInfo` here. This sounds like a more high level semantical check, while checks in Resources::validate are all quite low level (e.g., check for invalid name, type name mismatch, etc.).
What do you think?
src/master/validation.cpp
<https://reviews.apache.org/r/32150/#comment128812>
We typically uses single quote in our code base:
```
The reserved resource's role 'xxx' does not...
```
That can also help us avoid the escape char '\'.
src/master/validation.cpp
<https://reviews.apache.org/r/32150/#comment128820>
Is this necessary, or will be captured by the resources contains check later?
src/tests/master_validation_tests.cpp
<https://reviews.apache.org/r/32150/#comment128827>
Can you split this test into a few smaller tests (i.e., one for each {} block)? The comment in each {} block can be the test name. In that way, when a test fails, you immediately know what went wrong. What do you think?
src/tests/master_validation_tests.cpp
<https://reviews.apache.org/r/32150/#comment128824>
Can we get rid of this temp variable (to be consistent with the `"*"` case below)?
src/tests/master_validation_tests.cpp
<https://reviews.apache.org/r/32150/#comment128828>
Ditto on temp variable and splitting into smaller tests.
src/tests/master_validation_tests.cpp
<https://reviews.apache.org/r/32150/#comment128822>
Add quote to 'principal'.
```
// 'principal' matches.
```
- Jie Yu
On April 8, 2015, 5:04 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32150/
> -----------------------------------------------------------
>
> (Updated April 8, 2015, 5:04 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2139
> https://issues.apache.org/jira/browse/MESOS-2139
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled reservation operations in `Master::_accept`.
>
> Added `validate` functions in `src/master/validation.{hpp,cpp}`.
>
>
> Diffs
> -----
>
> src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b
> src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
> src/master/validation.cpp 2f2e4ea8ea123c5a0d01446cdec8b308ea60932e
> src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
>
> Diff: https://reviews.apache.org/r/32150/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>