----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70994/#review216358 -----------------------------------------------------------
Fix it, then Ship it! How about: ``` Extracted suppression logic in allocator for use in update framework. This patch moves the logic of suppressing/unsuppressing a role set from the inside of 'suppressOffers()'/'reviveOffers()' into separate methods. Specifically, 'reviveOffers()' includes filter clearing logic that we don't want when unsuppressing roles during framework update. For 'supppressOffers()', we need the empty set == all roles semantics, but we don't want that in the suppression logic during framework update. Longer term, the empty set == all roles semantics could be done in the master and we won't need the extra function to provide empty set == all roles logic in the allocator. This is a prerequisite for using thes methods to fix 'updateFramework()' in a subsequent patch. ``` Note that I think we don't need the extra suppression function in the allocator, however we need to update the master to provide the empty set == all roles semantics (which is more of a protobuf API concern than something the allocator should know about). src/master/allocator/mesos/hierarchical.cpp Line 1342 (original), 1342 (patched) <https://reviews.apache.org/r/70994/#comment303557> Why not take `Framework*` in both of these? ... Wow, in the process of looking into this, I see we don't store the FrameworkID inside the Framework struct.. we should change that, but for this patch taking FrameworkID is ok. Maybe some TODOs. src/master/allocator/mesos/hierarchical.cpp Lines 1363-1364 (original), 1375-1376 (patched) <https://reviews.apache.org/r/70994/#comment303558> Probably we should just log this in the suppressRoles function? Ideally, we only log those roles that transitioned from unsuppressed -> suppressed, but we currently log the no-ops for roles that are already unsuppressed. A TODO is fine for now: ``` // TODO(bmahler): This logs roles that were already suppressed, // only log roles that transitioned from unsuppressed -> suppressed. LOG(INFO) << "Suppressed offers for roles " << stringify(roles) << " of framework " << frameworkId; ``` src/master/allocator/mesos/hierarchical.cpp Line 1391 (original), 1398 (patched) <https://reviews.apache.org/r/70994/#comment303560> To be consistent: ``` // TODO(bmahler): This logs roles that were already unsuppressed, // only log roles that transitioned from suppressed -> unsuppressed. LOG(INFO) << "Unsuppressed offers for roles " << stringify(roles) << " of framework " << frameworkId; ``` reviveOffers can keep logging, since we also want to know when filters were cleared: ``` LOG(INFO) << "Revived roles " << stringify(roles) << " of framework " << frameworkId; ``` - Benjamin Mahler On July 2, 2019, 7:10 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70994/ > ----------------------------------------------------------- > > (Updated July 2, 2019, 7:10 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 logic of suppressing/unsuppressing a role set from > the inside of 'suppressOffers()'/'reviveOffers()' into separate methods. > > This is a prerequisite for using this methods to fix updateFramework() > in the dependent patch. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.hpp > 7e9765263f969a4499358579f1ee5bb1afb053da > src/master/allocator/mesos/hierarchical.cpp > 26aad6778f12b99bb87c846788d6b6d60f743d8a > > > Diff: https://reviews.apache.org/r/70994/diff/1/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > >
