Re: Review Request 70894: Provided ability to change suppressed roles via V0 updateFramework().

[Andrei Sekretenko <as...@mesosphere.io>]

> On June 25, 2019, 7:13 p.m., Benjamin Mahler wrote:
> > Thanks for the update. Why not update suppressOffers in this patch? What's the revert concern about? It seems we either need both updated in this patch or we need another patch for suppressOffers that immediately follows this. I would prefer to have this patch introduce the suppress roles set, which means updating reviveOffers/suppressOffers/updateFramework in this patch.

My initial concern was that some framework that depends on current behavior of `suppressOffers()` might turn up, and we will have to do something with this... 

Given that in a week's time noone raised such concerns, that the change needed to make `suppressOffers()` consistent with everything else is trivial, and that the implementation of per-role `suppressOffers()` heavily depends on this change anyway, I decided to update suppressOffers() here.


> On June 25, 2019, 7:13 p.m., Benjamin Mahler wrote:
> > include/mesos/scheduler.hpp
> > Lines 295-297 (patched)
> > <https://reviews.apache.org/r/70894/diff/2/?file=2152130#file2152130line295>
> >
> >     Hm.. I don't understand what this TODO is about

Dropped this TODO - the removal of offer filters has never been reliable in the V0 scheduler (like most other requests in V0), and this patch doesn't affect this.


> On June 25, 2019, 7:13 p.m., Benjamin Mahler wrote:
> > include/mesos/scheduler.hpp
> > Lines 303-307 (patched)
> > <https://reviews.apache.org/r/70894/diff/2/?file=2152130#file2152130line303>
> >
> >     This is a little vague, do you mean to have it clear the suppressed roles list? It seems you've already expressed in the review commentary that you are planning to update this in a separate patch?
> >     
> >     Why not make them consistent in this patch? At the very least I would want to commit them together, it seems inconsistent to only update reviveOffers to clear the supppressed set, but not suppressOffers?

Changed `suppressedOffers()` too - see above.


- Andrei


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70894/#review216124
-----------------------------------------------------------


On June 26, 2019, 3:28 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70894/
> -----------------------------------------------------------
> 
> (Updated June 26, 2019, 3:28 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9793
>     https://issues.apache.org/jira/browse/MESOS-9793
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch adds a list of suppressed roles to the arguments of
> scheduler driver's updateFramework() method to make it possible
> for V0 frameworks to selectively suppress/revive offers.
> 
> To keep re-registration conststent with updateFramework(), the required
> set of suppressed roles is now stored by the driver and used when it
> re-registers.
> 
> To keep reviveOffers() consistent with re-registration, reviveOffers()
> now clears the stored set and, when issued in a disconnected state of
> the driver, forces it to perform UPDATE_FRAMEWORK call upon
> re-connection.
> 
> NOTE: suppressOffers() has never been consistent with re-registration
> in this regard. Making it constent with re-registration might affect
> the behavior of existing frameworks, and will be performed in a separate
> patch.
> 
> 
> Diffs
> -----
> 
>   include/mesos/scheduler.hpp 8c6774885ceb3b0644c32842a17fba39e2c3e472 
>   src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp a21aca23cbef27b3c54bf1ae5834cbb457608130 
>   src/java/src/org/apache/mesos/SchedulerDriver.java ee5a9e24956299d84ff03a0fc5a22e641470a03f 
>   src/python/interface/src/mesos/interface/__init__.py f9642a0452e3161f3fd1b6a5d7ce0658d08c0b87 
>   src/sched/sched.cpp e6cc534264e3ff4edaa703a660afb2f896388802 
>   src/tests/master/update_framework_tests.cpp 0d466e286adfd829bbe72cff9202860f7fcb043f 
> 
> 
> Diff: https://reviews.apache.org/r/70894/diff/3/
> 
> 
> Testing
> -------
> 
> make check
> `./bin/mesos-tests.sh --gtest_filter="UpdateFrameworkV0*" --gtest_break_on_failure --gtest_repeat=1000`
> 
> +a new test from the depending patch
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>