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