Re: Review Request 32150: Enabled the master to handle reservation operations.
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: Enabled the master to handle reservation operations.
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 Optionstring
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 Optionstring
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:
```
OptionError error = validation::operation::validate(
operation.unreserve(),
(framework-info.has_principal() ?
framework-info.principal() :
Optionstring::none()));
```
as well as within the function: we pass an `Optionstring` 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 `Optionstring` here?
Removed the `snake_case`, please let me know about the issue around passing a
`bool` vs `Optionstring`.
- 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.
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.
---
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.
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:
```
Optionstring principal = framework-info.has_principal()
? framework-info.principal()
: Optionstring::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.
---
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.
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.
---
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:
```
OptionError error = validation::operation::validate(
operation.reserve(),
framework-info.role(),
(framework-info.has_principal() ?
framework-info.principal() :
Optionstring::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 Optionstring 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.
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:
```
OptionError error = validation::operation::validate(
operation.reserve(),
framework-info.role(),
(framework-info.has_principal() ?
framework-info.principal() :
Optionstring::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 Optionstring
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 Optionstring 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:
```
OptionError error = validation::operation::validate(
operation.unreserve(),
(framework-info.has_principal() ?
framework-info.principal() :
Optionstring::none()));
```
as well as within the function: we pass an `Optionstring` 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 `Optionstring` 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.
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.
---
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.
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.
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.
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
