Re: Review Request 70980: Removed deactivation of already inactive roles on framework update.

[Benjamin Mahler <bm...@apache.org>]

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