You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jay Guo <gu...@gmail.com> on 2017/02/24 10:48:04 UTC
Re: Review Request 56939: Added allocation role validation for
ExecutorInfo.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56939/
-----------------------------------------------------------
(Updated Feb. 24, 2017, 6:48 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and Michael Park.
Summary (updated)
-----------------
Added allocation role validation for ExecutorInfo.
Bugs: MESOS-6636
https://issues.apache.org/jira/browse/MESOS-6636
Repository: mesos
Description (updated)
-------
With support for multi-role frameworks, we need to make sure that
individual executors cannot mix allocation roles.
Diffs (updated)
-----
src/master/validation.hpp f03b3280704083be1ca074ca07c69edbb49dae19
src/master/validation.cpp b8c6b49ca5ca1f15e58a2967adc335436a35071f
src/tests/master_validation_tests.cpp 45254739e2a62fed7008ceada7a8f64bb9ee4e31
Diff: https://reviews.apache.org/r/56939/diff/
Testing
-------
make check
Thanks,
Jay Guo
Re: Review Request 56939: Added allocation role validation for
ExecutorInfo.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56939/#review166749
-----------------------------------------------------------
Fix it, then Ship it!
src/master/validation.cpp (lines 587 - 610)
<https://reviews.apache.org/r/56939/#comment238787>
How about we validate that it's allocated, and it's allocated to 1 role, so something like `validateAllocatedToSingleRole`.
src/master/validation.cpp (lines 602 - 604)
<https://reviews.apache.org/r/56939/#comment238781>
We try to open and close quotes on the same line, since when on different lines we sometimes forget to close them. E.g.
```
"The resources have multiple allocation roles ('" + _role + "'"
" and '" + role.get() + "') but only one allocation role" +
" is allowed");
```
or:
```
"The resources have multiple allocation roles"
" ('" + _role + "' and '" + role.get() + "')"
" but only one allocation role is allowed");
```
src/tests/master_validation_tests.cpp (line 2230)
<https://reviews.apache.org/r/56939/#comment238786>
We should also test the case where the executor has unallocated resources, which is not allowed since the master ensures the allocation info is injected. At this point, maybe a different test name, like `ExecutorInfoAllocatedResources`.
src/tests/master_validation_tests.cpp (lines 2235 - 2236)
<https://reviews.apache.org/r/56939/#comment238838>
Hm.. we should probably just pull this test out into its own scope?
- Benjamin Mahler
On Feb. 24, 2017, 10:48 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56939/
> -----------------------------------------------------------
>
> (Updated Feb. 24, 2017, 10:48 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and Michael Park.
>
>
> Bugs: MESOS-6636
> https://issues.apache.org/jira/browse/MESOS-6636
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With support for multi-role frameworks, we need to make sure that
> individual executors cannot mix allocation roles.
>
>
> Diffs
> -----
>
> src/master/validation.hpp f03b3280704083be1ca074ca07c69edbb49dae19
> src/master/validation.cpp b8c6b49ca5ca1f15e58a2967adc335436a35071f
> src/tests/master_validation_tests.cpp 45254739e2a62fed7008ceada7a8f64bb9ee4e31
>
> Diff: https://reviews.apache.org/r/56939/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 56939: Added allocation role validation for
ExecutorInfo.
Posted by Jay Guo <gu...@gmail.com>.
> On Feb. 24, 2017, 6:51 p.m., Jay Guo wrote:
> > src/master/validation.cpp, lines 590-609
> > <https://reviews.apache.org/r/56939/diff/2/?file=1646991#file1646991line590>
> >
> > Could we leverage `Resources::allocations()` to help with mix detection? e.g.
> > ```
> > If (resources.allocations().size() > 1) {
> > return Error(...);
> > }
> > ```
> >
> > In this case, `RESERVE` operation could not use this method since resources are not always allocated in case we are dealing with Operator API.
Also, should we deal with the case where some of the resources are _not_ allocated, and others are allocated to same role.
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56939/#review166680
-----------------------------------------------------------
On Feb. 24, 2017, 6:48 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56939/
> -----------------------------------------------------------
>
> (Updated Feb. 24, 2017, 6:48 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and Michael Park.
>
>
> Bugs: MESOS-6636
> https://issues.apache.org/jira/browse/MESOS-6636
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With support for multi-role frameworks, we need to make sure that
> individual executors cannot mix allocation roles.
>
>
> Diffs
> -----
>
> src/master/validation.hpp f03b3280704083be1ca074ca07c69edbb49dae19
> src/master/validation.cpp b8c6b49ca5ca1f15e58a2967adc335436a35071f
> src/tests/master_validation_tests.cpp 45254739e2a62fed7008ceada7a8f64bb9ee4e31
>
> Diff: https://reviews.apache.org/r/56939/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 56939: Added allocation role validation for
ExecutorInfo.
Posted by Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56939/#review166680
-----------------------------------------------------------
src/master/validation.cpp (lines 590 - 609)
<https://reviews.apache.org/r/56939/#comment238704>
Could we leverage `Resources::allocations()` to help with mix detection? e.g.
```
If (resources.allocations().size() > 1) {
return Error(...);
}
```
In this case, `RESERVE` operation could not use this method since resources are not always allocated in case we are dealing with Operator API.
- Jay Guo
On Feb. 24, 2017, 6:48 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56939/
> -----------------------------------------------------------
>
> (Updated Feb. 24, 2017, 6:48 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and Michael Park.
>
>
> Bugs: MESOS-6636
> https://issues.apache.org/jira/browse/MESOS-6636
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With support for multi-role frameworks, we need to make sure that
> individual executors cannot mix allocation roles.
>
>
> Diffs
> -----
>
> src/master/validation.hpp f03b3280704083be1ca074ca07c69edbb49dae19
> src/master/validation.cpp b8c6b49ca5ca1f15e58a2967adc335436a35071f
> src/tests/master_validation_tests.cpp 45254739e2a62fed7008ceada7a8f64bb9ee4e31
>
> Diff: https://reviews.apache.org/r/56939/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>