You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Guangya Liu <gy...@apache.org> on 2017/02/06 15:13:13 UTC
Review Request 56330: Enabled suppress offer per role.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56330/
-----------------------------------------------------------
Review request for mesos, Benjamin Mahler and Jay Guo.
Bugs: MESOS-6638
https://issues.apache.org/jira/browse/MESOS-6638
Repository: mesos
Description
-------
Enabled suppress offer per role.
Diffs
-----
include/mesos/allocator/allocator.hpp 71a40537b673e44ecdd5327d9a9f083faa7fc13a
src/master/allocator/mesos/allocator.hpp e3c86181677302dbfc3b213715196122f96e312c
src/master/allocator/mesos/hierarchical.hpp 896abcdf0727f986eef3a1a9304a0e4847094057
src/master/allocator/mesos/hierarchical.cpp 3429a6591e5041240736dd033acc23253d9942c8
src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a
src/tests/allocator.hpp 32c291213d18d1c8fe5d9e8194b92c10716b9961
src/tests/hierarchical_allocator_tests.cpp c681d03c3f94f7d071143366a5aad0421108ebec
Diff: https://reviews.apache.org/r/56330/diff/
Testing
-------
make
make check
Will add a test case to enable suppress per role in follow up patches.
Thanks,
Guangya Liu
Re: Review Request 56330: Enabled suppress offer per role.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56330/#review164356
-----------------------------------------------------------
Patch looks great!
Reviews applied: [56327, 56328, 56330]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 6, 2017, 3:13 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56330/
> -----------------------------------------------------------
>
> (Updated Feb. 6, 2017, 3:13 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Jay Guo.
>
>
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Enabled suppress offer per role.
>
>
> Diffs
> -----
>
> include/mesos/allocator/allocator.hpp 71a40537b673e44ecdd5327d9a9f083faa7fc13a
> src/master/allocator/mesos/allocator.hpp e3c86181677302dbfc3b213715196122f96e312c
> src/master/allocator/mesos/hierarchical.hpp 896abcdf0727f986eef3a1a9304a0e4847094057
> src/master/allocator/mesos/hierarchical.cpp 3429a6591e5041240736dd033acc23253d9942c8
> src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a
> src/tests/allocator.hpp 32c291213d18d1c8fe5d9e8194b92c10716b9961
> src/tests/hierarchical_allocator_tests.cpp c681d03c3f94f7d071143366a5aad0421108ebec
>
> Diff: https://reviews.apache.org/r/56330/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> Will add a test case to enable suppress per role in follow up patches.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 56330: Enabled suppress offer per role.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Feb. 6, 2017, 9:15 p.m., Benjamin Mahler wrote:
> > include/mesos/allocator/allocator.hpp, lines 352-353
> > <https://reviews.apache.org/r/56330/diff/1/?file=1624787#file1624787line352>
> >
> > Hm.. it doesn't look like any of the comments in this header make use of @param.
>
> Guangya Liu wrote:
> Do you mean we do not need `@param`? But there are actually many places using such format as https://github.com/apache/mesos/blob/master/include/mesos/allocator/allocator.hpp#L76-L86 , comments?
Ah I see now, sounds good I will drop this.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56330/#review164407
-----------------------------------------------------------
On Feb. 7, 2017, 10:10 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56330/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2017, 10:10 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Jay Guo.
>
>
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Enabled suppress offer per role.
>
>
> Diffs
> -----
>
> include/mesos/allocator/allocator.hpp 71a40537b673e44ecdd5327d9a9f083faa7fc13a
> src/master/allocator/mesos/allocator.hpp e3c86181677302dbfc3b213715196122f96e312c
> src/master/allocator/mesos/hierarchical.hpp 896abcdf0727f986eef3a1a9304a0e4847094057
> src/master/allocator/mesos/hierarchical.cpp 56d6791baa64189523df668749f4a7ab67d6b363
> src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a
> src/tests/allocator.hpp 32c291213d18d1c8fe5d9e8194b92c10716b9961
> src/tests/hierarchical_allocator_tests.cpp c681d03c3f94f7d071143366a5aad0421108ebec
>
> Diff: https://reviews.apache.org/r/56330/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> Will add a test case to enable suppress per role in follow up patches.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 56330: Enabled suppress offer per role.
Posted by Guangya Liu <gy...@apache.org>.
> On \u4e8c\u6708 6, 2017, 9:15 p.m., Benjamin Mahler wrote:
> > include/mesos/allocator/allocator.hpp, lines 352-353
> > <https://reviews.apache.org/r/56330/diff/1/?file=1624787#file1624787line352>
> >
> > Hm.. it doesn't look like any of the comments in this header make use of @param.
Do you mean we do not need `@param`? But there are actually many places using such format as https://github.com/apache/mesos/blob/master/include/mesos/allocator/allocator.hpp#L76-L86 , comments?
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56330/#review164407
-----------------------------------------------------------
On \u4e8c\u6708 7, 2017, 10:10 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56330/
> -----------------------------------------------------------
>
> (Updated \u4e8c\u6708 7, 2017, 10:10 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Jay Guo.
>
>
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Enabled suppress offer per role.
>
>
> Diffs
> -----
>
> include/mesos/allocator/allocator.hpp 71a40537b673e44ecdd5327d9a9f083faa7fc13a
> src/master/allocator/mesos/allocator.hpp e3c86181677302dbfc3b213715196122f96e312c
> src/master/allocator/mesos/hierarchical.hpp 896abcdf0727f986eef3a1a9304a0e4847094057
> src/master/allocator/mesos/hierarchical.cpp 56d6791baa64189523df668749f4a7ab67d6b363
> src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a
> src/tests/allocator.hpp 32c291213d18d1c8fe5d9e8194b92c10716b9961
> src/tests/hierarchical_allocator_tests.cpp c681d03c3f94f7d071143366a5aad0421108ebec
>
> Diff: https://reviews.apache.org/r/56330/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> Will add a test case to enable suppress per role in follow up patches.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 56330: Enabled suppress offer per role.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56330/#review164407
-----------------------------------------------------------
include/mesos/allocator/allocator.hpp (lines 352 - 353)
<https://reviews.apache.org/r/56330/#comment236106>
Hm.. it doesn't look like any of the comments in this header make use of @param.
src/master/allocator/mesos/hierarchical.cpp (lines 1177 - 1191)
<https://reviews.apache.org/r/56330/#comment236105>
How about something like:
```
const set<string>& roles =
role.isSome() ? {role.get()} : framework.roles;
foreach (const string& role, roles) {
...
}
```
src/master/allocator/mesos/hierarchical.cpp (line 1178)
<https://reviews.apache.org/r/56330/#comment236103>
This CHECK can fail if the frameworks sets a Suppress.role to something not present in their FrameworkInfo.roles or FrameworkInfo.role fields.
We have two options:
(1) Drop it here with a warning when the role is not one of the framework's roles.
(2) Keep the CHECK but have the master validate that Suppress.role is one of the frameworks roles. If not, the master drops it as an invalid call.
The right approach here seems to be (2) as it's consistent with how we handle other calls.
- Benjamin Mahler
On Feb. 6, 2017, 3:13 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56330/
> -----------------------------------------------------------
>
> (Updated Feb. 6, 2017, 3:13 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Jay Guo.
>
>
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Enabled suppress offer per role.
>
>
> Diffs
> -----
>
> include/mesos/allocator/allocator.hpp 71a40537b673e44ecdd5327d9a9f083faa7fc13a
> src/master/allocator/mesos/allocator.hpp e3c86181677302dbfc3b213715196122f96e312c
> src/master/allocator/mesos/hierarchical.hpp 896abcdf0727f986eef3a1a9304a0e4847094057
> src/master/allocator/mesos/hierarchical.cpp 3429a6591e5041240736dd033acc23253d9942c8
> src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a
> src/tests/allocator.hpp 32c291213d18d1c8fe5d9e8194b92c10716b9961
> src/tests/hierarchical_allocator_tests.cpp c681d03c3f94f7d071143366a5aad0421108ebec
>
> Diff: https://reviews.apache.org/r/56330/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> Will add a test case to enable suppress per role in follow up patches.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 56330: Enabled suppress offer per role.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Feb. 8, 2017, 9:44 p.m., Benjamin Mahler wrote:
> > include/mesos/allocator/allocator.hpp, lines 352-353
> > <https://reviews.apache.org/r/56330/diff/2/?file=1626019#file1626019line352>
> >
> > This header doesn't use @param, we can just update the comment above:
> >
> > ```
> > Informs the allocator to stop sending offers to this framework for the specified role. If the role is not specified, we will stop sending offers to this framework for all of its roles.
> > ```
Feel free to use @param, but be sure to update the overall comment for the function.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56330/#review164774
-----------------------------------------------------------
On Feb. 7, 2017, 10:10 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56330/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2017, 10:10 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Jay Guo.
>
>
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Enabled suppress offer per role.
>
>
> Diffs
> -----
>
> include/mesos/allocator/allocator.hpp 71a40537b673e44ecdd5327d9a9f083faa7fc13a
> src/master/allocator/mesos/allocator.hpp e3c86181677302dbfc3b213715196122f96e312c
> src/master/allocator/mesos/hierarchical.hpp 896abcdf0727f986eef3a1a9304a0e4847094057
> src/master/allocator/mesos/hierarchical.cpp 56d6791baa64189523df668749f4a7ab67d6b363
> src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a
> src/tests/allocator.hpp 32c291213d18d1c8fe5d9e8194b92c10716b9961
> src/tests/hierarchical_allocator_tests.cpp c681d03c3f94f7d071143366a5aad0421108ebec
>
> Diff: https://reviews.apache.org/r/56330/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> Will add a test case to enable suppress per role in follow up patches.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 56330: Enabled suppress offer per role.
Posted by Guangya Liu <gy...@apache.org>.
> On \u4e8c\u6708 8, 2017, 9:44 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp, lines 3230-3236
> > <https://reviews.apache.org/r/56330/diff/2/?file=1626023#file1626023line3230>
> >
> > Why do we need to validate the role? It seems sufficient to just check whether it is one of the framework's roles since all of the framework roles are valid.
Yes, all of the frameworks roles are valid here, but there maybe cases that the framework developer set an `invalid role` when setting the `Call:SUPPRESS` in the framework?
I updated the comments a bit here:
```
// There maybe cases that the framework developer set an invalid role
// when constructing `scheduler::Call::Suppress`.
```
> On \u4e8c\u6708 8, 2017, 9:44 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp, lines 3238-3246
> > <https://reviews.apache.org/r/56330/diff/2/?file=1626023#file1626023line3238>
> >
> > " because it is not one of the framework's subscribed roles"
> >
> > I would suggest two follow ups patches here:
> >
> > (1) Let's store the roles set within the Framework struct, so that we don't have to keep re-computing it and can just write:
> >
> > ```
> > if (framework->roles.count(role.get()) == 0) {
> > ...
> > }
> > ```
> >
> > (2) Add a drop overload to avoid custom logging here:
> >
> > ```
> > drop(framework,
> > suppress,
> > "suppression role ' + role.get() + " is not one"
> > " of the frameworks's subscribed roles");
> > ```
Added some `TODO` here to follow up later.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56330/#review164774
-----------------------------------------------------------
On \u4e8c\u6708 7, 2017, 10:10 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56330/
> -----------------------------------------------------------
>
> (Updated \u4e8c\u6708 7, 2017, 10:10 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Jay Guo.
>
>
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Enabled suppress offer per role.
>
>
> Diffs
> -----
>
> include/mesos/allocator/allocator.hpp 71a40537b673e44ecdd5327d9a9f083faa7fc13a
> src/master/allocator/mesos/allocator.hpp e3c86181677302dbfc3b213715196122f96e312c
> src/master/allocator/mesos/hierarchical.hpp 896abcdf0727f986eef3a1a9304a0e4847094057
> src/master/allocator/mesos/hierarchical.cpp 56d6791baa64189523df668749f4a7ab67d6b363
> src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a
> src/tests/allocator.hpp 32c291213d18d1c8fe5d9e8194b92c10716b9961
> src/tests/hierarchical_allocator_tests.cpp c681d03c3f94f7d071143366a5aad0421108ebec
>
> Diff: https://reviews.apache.org/r/56330/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> Will add a test case to enable suppress per role in follow up patches.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 56330: Enabled suppress offer per role.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56330/#review164774
-----------------------------------------------------------
Fix it, then Ship it!
include/mesos/allocator/allocator.hpp (lines 352 - 353)
<https://reviews.apache.org/r/56330/#comment236543>
This header doesn't use @param, we can just update the comment above:
```
Informs the allocator to stop sending offers to this framework for the specified role. If the role is not specified, we will stop sending offers to this framework for all of its roles.
```
src/master/allocator/mesos/hierarchical.cpp (line 1182)
<https://reviews.apache.org/r/56330/#comment236545>
Feel free to just do s/role(s)/roles/, no need to bother with this IMHO.
src/master/master.cpp (lines 3230 - 3236)
<https://reviews.apache.org/r/56330/#comment236533>
Why do we need to validate the role? It seems sufficient to just check whether it is one of the framework's roles since all of the framework roles are valid.
src/master/master.cpp (lines 3238 - 3246)
<https://reviews.apache.org/r/56330/#comment236541>
" because it is not one of the framework's subscribed roles"
I would suggest two follow ups patches here:
(1) Let's store the roles set within the Framework struct, so that we don't have to keep re-computing it and can just write:
```
if (framework->roles.count(role.get()) == 0) {
...
}
```
(2) Add a drop overload to avoid custom logging here:
```
drop(framework,
suppress,
"suppression role ' + role.get() + " is not one"
" of the frameworks's subscribed roles");
```
- Benjamin Mahler
On Feb. 7, 2017, 10:10 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56330/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2017, 10:10 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Jay Guo.
>
>
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Enabled suppress offer per role.
>
>
> Diffs
> -----
>
> include/mesos/allocator/allocator.hpp 71a40537b673e44ecdd5327d9a9f083faa7fc13a
> src/master/allocator/mesos/allocator.hpp e3c86181677302dbfc3b213715196122f96e312c
> src/master/allocator/mesos/hierarchical.hpp 896abcdf0727f986eef3a1a9304a0e4847094057
> src/master/allocator/mesos/hierarchical.cpp 56d6791baa64189523df668749f4a7ab67d6b363
> src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a
> src/tests/allocator.hpp 32c291213d18d1c8fe5d9e8194b92c10716b9961
> src/tests/hierarchical_allocator_tests.cpp c681d03c3f94f7d071143366a5aad0421108ebec
>
> Diff: https://reviews.apache.org/r/56330/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> Will add a test case to enable suppress per role in follow up patches.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 56330: Enabled suppress offer per role.
Posted by Guangya Liu <gy...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56330/
-----------------------------------------------------------
(Updated \u4e8c\u6708 9, 2017, 6:05 a.m.)
Review request for mesos, Benjamin Mahler and Jay Guo.
Bugs: MESOS-6638
https://issues.apache.org/jira/browse/MESOS-6638
Repository: mesos
Description
-------
Enabled suppress offer per role.
Diffs (updated)
-----
include/mesos/allocator/allocator.hpp 71a40537b673e44ecdd5327d9a9f083faa7fc13a
src/master/allocator/mesos/allocator.hpp e3c86181677302dbfc3b213715196122f96e312c
src/master/allocator/mesos/hierarchical.hpp 896abcdf0727f986eef3a1a9304a0e4847094057
src/master/allocator/mesos/hierarchical.cpp 017253cc8f5fbcccd9d4057c5b189f352779513c
src/master/master.cpp 0b65345d48192a1536d43973cf782ade3c1c8163
src/tests/allocator.hpp 32c291213d18d1c8fe5d9e8194b92c10716b9961
src/tests/hierarchical_allocator_tests.cpp c681d03c3f94f7d071143366a5aad0421108ebec
Diff: https://reviews.apache.org/r/56330/diff/
Testing
-------
make
make check
Will add a test case to enable suppress per role in follow up patches.
Thanks,
Guangya Liu
Re: Review Request 56330: Enabled suppress offer per role.
Posted by Guangya Liu <gy...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56330/
-----------------------------------------------------------
(Updated \u4e8c\u6708 7, 2017, 10:10 a.m.)
Review request for mesos, Benjamin Mahler and Jay Guo.
Bugs: MESOS-6638
https://issues.apache.org/jira/browse/MESOS-6638
Repository: mesos
Description
-------
Enabled suppress offer per role.
Diffs (updated)
-----
include/mesos/allocator/allocator.hpp 71a40537b673e44ecdd5327d9a9f083faa7fc13a
src/master/allocator/mesos/allocator.hpp e3c86181677302dbfc3b213715196122f96e312c
src/master/allocator/mesos/hierarchical.hpp 896abcdf0727f986eef3a1a9304a0e4847094057
src/master/allocator/mesos/hierarchical.cpp 56d6791baa64189523df668749f4a7ab67d6b363
src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a
src/tests/allocator.hpp 32c291213d18d1c8fe5d9e8194b92c10716b9961
src/tests/hierarchical_allocator_tests.cpp c681d03c3f94f7d071143366a5aad0421108ebec
Diff: https://reviews.apache.org/r/56330/diff/
Testing
-------
make
make check
Will add a test case to enable suppress per role in follow up patches.
Thanks,
Guangya Liu