----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70980/#review216281 -----------------------------------------------------------
Since there are a lot of intermediate reviews here, I just reviewed the final state of the code here, see below: src/master/allocator/mesos/hierarchical.cpp Lines 496-505 (original), 496-505 (patched) <https://reviews.apache.org/r/70980/#comment303424> The distinction between rolesToSuppress/rolesToRevive and rolesToActivate/rolesToDeactivate is confusing (they sound the same). And the distinction between the logical blocks that use them is also strange (since only metrics are being touched for the former sets): - rolesToSuppress/rolesToRevive are not the roles to suppress or revive, they are just the roles that have been added or removed from the field (which is quite different!) - rolesToActivate and rolesToDeactivate are also confusing because there is overlap with rolesToAdd and rolesToRemove - Let's reuse our existing suppress / revive logic (and make any adjustments if needed to make it work) Perhaps some logic like this? ``` for each role to add: add role for each role to remove: remove role framework.roles = new roles suppressOffers(framework.id, new suppressed roles) reviveOffers(framework.id, framework.roles - new suppressed roles) ``` suppressOffers and reviveOffers will already adjust framework.suppressedRoles and will avoid ?all? of the confusing set stuff here We'll have to add an unsuppress function FWICT: ``` for each role to add: add role for each role to remove: remove role framework.roles = new roles suppressOffers(framework.id, new suppressed roles) unsuppressOffers(framework.id, framework.roles - new suppressed roles) // For roles that just moved out of being suppressed, we revive them: reviveOffers(framework.id, framework.roles & (old suppressed - new suppressed)) ``` - Benjamin Mahler On June 29, 2019, 12:56 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70980/ > ----------------------------------------------------------- > > (Updated June 29, 2019, 12:56 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 is a refactoring of `updateFramework()` in the hierarchial > allocator which makes the symmetry between activating and > deactivating roles more visible. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.cpp > 26aad6778f12b99bb87c846788d6b6d60f743d8a > > > Diff: https://reviews.apache.org/r/70980/diff/1/ > > > Testing > ------- > > make check > > > Thanks, > > Andrei Sekretenko > >
