> On June 25, 2019, 7:13 p.m., Benjamin Mahler wrote: > > include/mesos/scheduler.hpp > > Lines 290-293 (patched) > > <https://reviews.apache.org/r/70894/diff/2/?file=2152130#file2152130line290> > > > > // NOTE: if the framework is not connected to the master, the suppressed > > // roles set stored by the driver will be cleared and the next > > re-registration > > // will send an empty suppressed roles list.
Thanks! Fully agree that I need a better wording here. I'm afraid these two phrases might mislead someone someday: - "next re-registration": it might be not clear to the reader if the ongoing re-registrtation (SUBSCRIBE sent, but no FrameworkReregistered received) is the "next" one or not. - "will send an empty list": if, for example, updateFramework()/suppress() is called immediately after reviveOffers(), the set which the driver will set will be what the caller wants - but it will not be empty. So I've tried to reword the comment (and remove implementation details) without using these phrases. > On June 25, 2019, 7:13 p.m., Benjamin Mahler wrote: > > include/mesos/scheduler.hpp > > Lines 352 (patched) > > <https://reviews.apache.org/r/70894/diff/2/?file=2152130#file2152130line352> > > > > Why the default here? My impression at the moment of writing that was that updateFramework() will not be frequently called with non-empty suppressedRoles - however, now this impression looks totally wrong. I've removed this default. Combination of default and virtual method might be confusing, and, more importantly, this default does not help readers be aware of the fact that updateFramework() always sets suppressed roles. - Andrei ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70894/#review216124 ----------------------------------------------------------- On June 25, 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 25, 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/src/org/apache/mesos/SchedulerDriver.java > ee5a9e24956299d84ff03a0fc5a22e641470a03f > src/python/interface/src/mesos/interface/__init__.py > f9642a0452e3161f3fd1b6a5d7ce0658d08c0b87 > src/sched/sched.cpp e6cc534264e3ff4edaa703a660afb2f896388802 > > > Diff: https://reviews.apache.org/r/70894/diff/2/ > > > 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 > >
