> 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 > >
