----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70967/#review216213 -----------------------------------------------------------
Looks good, just some minor notes below. src/master/allocator/mesos/hierarchical.cpp Lines 488-505 (original), 488-522 (patched) <https://reviews.apache.org/r/70967/#comment303312> Have you considered implementing the TODO to tidy this up? ``` const set<string> removedRoles = oldRoles - newRoles; const set<string> addedRoles = newRoles - oldRoles; const set<string> newSuppressedRoles = suppressedRoles - oldSuppressedRoles; const set<string> oldSuppressedRoles = oldSuppressedRoles - suppressedRoles; ``` Needs adding a function here that uses std::set_difference: https://github.com/apache/mesos/blob/1.8.0/3rdparty/stout/include/stout/set.hpp Of course, feel free to do this after the fix. src/master/allocator/mesos/hierarchical.cpp Lines 498 (patched) <https://reviews.apache.org/r/70967/#comment303310> No need for this note, it's obvious? (whereas the latter note is nice since there is something suprising, see below) src/master/allocator/mesos/hierarchical.cpp Lines 515 (patched) <https://reviews.apache.org/r/70967/#comment303311> This is actually suprising, a role that is removed from both `FramworkInfo.roles` and `suppressedRoles` is not being unsuppressed, so why store it in the set? Looking at your next patch, it looks like we indeed skip any entries that are also in `removedRoles` anyway? Might as well not include them in the set? If you'd like to keep the this patch as purely cleanup, perhaps make this NOTE a TODO to not consider removed roles to be unsuppressed, and have the next patch make that adjustment? - Benjamin Mahler On June 27, 2019, 7:37 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70967/ > ----------------------------------------------------------- > > (Updated June 27, 2019, 7:37 p.m.) > > > Review request for mesos, Benjamin Mahler and Meng Zhu. > > > Bugs: MESOS-9870 > https://issues.apache.org/jira/browse/MESOS-9870 > > > Repository: mesos > > > Description > ------- > > This patch moves the state-manipulating bits closer together, replaces > the misleading NewRevivedRoles name with NewUnsuppressedRoles and adds > const qualifiers to locals. > > This is a prerequisite to fix of MESOS-9870 bug in the next patch. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.cpp > 26aad6778f12b99bb87c846788d6b6d60f743d8a > > > Diff: https://reviews.apache.org/r/70967/diff/1/ > > > Testing > ------- > > make check > > > Thanks, > > Andrei Sekretenko > >
