mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Guangya Liu <gy...@apache.org>
Subject Re: Review Request 56330: Enabled suppress offer per role.
Date Thu, 09 Feb 2017 06:05:41 GMT


> On 二月 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 二月 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 二月 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 二月 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message