You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrei Sekretenko <as...@mesosphere.io> on 2019/06/19 18:26:27 UTC
Review Request 70894: Provided ability to change suppressed roles via
V0 updateFramework().
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70894/
-----------------------------------------------------------
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.
Diffs
-----
include/mesos/scheduler.hpp 8c6774885ceb3b0644c32842a17fba39e2c3e472
src/sched/sched.cpp e6cc534264e3ff4edaa703a660afb2f896388802
Diff: https://reviews.apache.org/r/70894/diff/1/
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
Re: Review Request 70894: Provided ability to change suppressed roles
via V0 updateFramework().
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70894/#review216293
-----------------------------------------------------------
Fix it, then Ship it!
The NOTE in the commit description looks stale? Here's an edit:
```
Provided ability to pass suppressed roles via V0 updateFramework().
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 reach
parity with V1 UPDATE_FRAMEWORK.
To keep re-registration consistent with updateFramework(), the set
of suppressed roles is now stored by the driver and used when it
re-registers.
To prevent re-registration from implicitly altering the effects of
reviveOffers() and suppressOffers(), reviveOffers() now clears the
stored suppressed roles and suppressOffers() now fills the stored
suppressed roles. If these calls are issued in a disconnected state
of the driver, the driver will perform an UPDATE_FRAMEWORK call upon
re-connection.
```
include/mesos/scheduler.hpp
Lines 293 (patched)
<https://reviews.apache.org/r/70894/#comment303467>
s/the //
include/mesos/scheduler.hpp
Lines 299 (patched)
<https://reviews.apache.org/r/70894/#comment303466>
s/\/re-registering with a new driver//
include/mesos/scheduler.hpp
Lines 304 (patched)
<https://reviews.apache.org/r/70894/#comment303468>
s/the //
src/java/src/org/apache/mesos/SchedulerDriver.java
Lines 263 (patched)
<https://reviews.apache.org/r/70894/#comment303469>
s/the //
src/java/src/org/apache/mesos/SchedulerDriver.java
Lines 275 (patched)
<https://reviews.apache.org/r/70894/#comment303470>
s/\/re-registering with a new driver//
src/java/src/org/apache/mesos/SchedulerDriver.java
Lines 280 (patched)
<https://reviews.apache.org/r/70894/#comment303471>
s/the //
src/python/interface/src/mesos/interface/__init__.py
Lines 249 (patched)
<https://reviews.apache.org/r/70894/#comment303472>
s/the //
src/python/interface/src/mesos/interface/__init__.py
Lines 257 (patched)
<https://reviews.apache.org/r/70894/#comment303473>
s/\/re-registering with a new driver//
src/python/interface/src/mesos/interface/__init__.py
Lines 262 (patched)
<https://reviews.apache.org/r/70894/#comment303474>
s/the //
src/sched/sched.cpp
Lines 1446 (patched)
<https://reviews.apache.org/r/70894/#comment303459>
s/the //
src/sched/sched.cpp
Lines 1447 (patched)
<https://reviews.apache.org/r/70894/#comment303460>
newline
src/sched/sched.cpp
Lines 1451 (patched)
<https://reviews.apache.org/r/70894/#comment303462>
s/ a//
src/sched/sched.cpp
Line 1441 (original), 1452 (patched)
<https://reviews.apache.org/r/70894/#comment303458>
newline
src/sched/sched.cpp
Lines 1453-1458 (original), 1467-1478 (patched)
<https://reviews.apache.org/r/70894/#comment303461>
we should make the newlines consistent with reviveOffers above
src/sched/sched.cpp
Lines 1470 (patched)
<https://reviews.apache.org/r/70894/#comment303464>
s/the //
src/sched/sched.cpp
Lines 1476 (patched)
<https://reviews.apache.org/r/70894/#comment303463>
s/a //
src/sched/sched.cpp
Line 2359 (original), 2388-2389 (patched)
<https://reviews.apache.org/r/70894/#comment303465>
It's a little odd that we silently ignore duplicates here, perhaps we should:
```
CHECK_EQ(suppressedRoles_.size(), suppressedRoles.size()) << "Invalid suppressed role list: contains " << suppressedRoles_.size() - suppressedRoles.size() << " duplicates " << suppressedRoles_;
```
- Benjamin Mahler
On July 1, 2019, 12:45 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70894/
> -----------------------------------------------------------
>
> (Updated July 1, 2019, 12:45 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/4/
>
>
> 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
>
>
Re: Review Request 70894: Provided ability to change suppressed roles
via V0 updateFramework().
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70894/
-----------------------------------------------------------
(Updated July 1, 2019, 12:45 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 (updated)
-----
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/4/
Changes: https://reviews.apache.org/r/70894/diff/3-4/
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
Re: Review Request 70894: Provided ability to change suppressed roles
via V0 updateFramework().
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/70894/diff/2-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
Re: Review Request 70894: Provided ability to change suppressed roles
via V0 updateFramework().
Posted by Andrei Sekretenko <as...@mesosphere.io>.
> 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
>
>
Re: Review Request 70894: Provided ability to change suppressed roles
via V0 updateFramework().
Posted by Andrei Sekretenko <as...@mesosphere.io>.
> 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.
>
> Andrei Sekretenko wrote:
> 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.
typo: s/the set which the driver will **set**/the set which the driver will **send**/
- 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
>
>
Re: Review Request 70894: Provided ability to change suppressed roles
via V0 updateFramework().
Posted by 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
>
>
Re: Review Request 70894: Provided ability to change suppressed roles
via V0 updateFramework().
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70894/#review216124
-----------------------------------------------------------
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.
include/mesos/scheduler.hpp
Lines 290-293 (patched)
<https://reviews.apache.org/r/70894/#comment303123>
// 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.
include/mesos/scheduler.hpp
Lines 295-297 (patched)
<https://reviews.apache.org/r/70894/#comment303124>
Hm.. I don't understand what this TODO is about
include/mesos/scheduler.hpp
Lines 303-307 (patched)
<https://reviews.apache.org/r/70894/#comment303126>
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?
include/mesos/scheduler.hpp
Lines 352 (patched)
<https://reviews.apache.org/r/70894/#comment303127>
Why the default here?
include/mesos/scheduler.hpp
Lines 492 (patched)
<https://reviews.apache.org/r/70894/#comment303128>
Ditto here, why the default?
src/java/src/org/apache/mesos/SchedulerDriver.java
Lines 260-263 (patched)
<https://reviews.apache.org/r/70894/#comment303129>
Ditto here
src/python/interface/src/mesos/interface/__init__.py
Lines 246-249 (patched)
<https://reviews.apache.org/r/70894/#comment303130>
Ditto here
src/sched/sched.cpp
Lines 1441 (patched)
<https://reviews.apache.org/r/70894/#comment303121>
No periods in log messages.
Looks like other messages of this nature are at VLOG(2)?
Can you make it consistent with other logging below?
```
void reviveOffers()
{
suppressedRoles.clear();
if (!connected) {
VLOG(1) << "Ignoring REVIVE as master is disconnected: "
<< "The scheduler will have all roles unsuppressed during"
<< " the next re-registration";
sendUpdateFrameworkOnConnect = true;
return;
}
VLOG(2) << "Asked to REVIVE for all roles";
```
src/sched/sched.cpp
Lines 1446 (patched)
<https://reviews.apache.org/r/70894/#comment303122>
REVIVE
disconected: An ...
also prefer spaces at the start of the line rather than the end of the line, makes it easier to read that spaces weren't missed
- Benjamin Mahler
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
>
>
Re: Review Request 70894: Provided ability to change suppressed roles
via V0 updateFramework().
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
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 (updated)
-------
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/70894/diff/1-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
Re: Review Request 70894: Provided ability to change suppressed roles
via V0 updateFramework().
Posted by Andrei Sekretenko <as...@mesosphere.io>.
> On June 19, 2019, 10:54 p.m., Benjamin Mahler wrote:
> > Hm.. there's a relationship between the 'suppressedRoles' list and the reviveOffers() / suppressOffers() calls.
> >
> > For example, if I call reviveOffers(), I would then expect that 'suppressedRoles' will be cleared and won't be sent if a re-registration happens under the covers due to a disconnection, e.g.
> >
> > ```
> > updateFramework(f, ["role1", "role2"]); // UPDATE_FRAMEWORK is sent
> > reviveOffers(); // REVIVE is sent, logically the master moves the suppressed roles from ["role1", "role2"] to []!
> > // disconnection and re-registration happens
> > // at this point, the driver should send [] as suppressed, since the scheduler unsuppressed all roles
> > // (but it sends ["role1", "role2"] in this patch)
> > ```
> >
> > Similarly for suppressOffers() where all of the roles become suppressed.
> >
> > If we add role(s) arguments to reviveOffers() / suppressOffers() then they would be removing items from the suppressed set rather than clearing and assigning it entirely.
Good point.
Now I'm wondering if the already existing behaviour of suppressOffers() + reconnection is valid.
Consder a similar chain of events:
- framework starts the driver
- framework calls suppressOffers()
- due to some reason, driver reconnects (to the same master or to the new one), all roles are active again
Is this a desired behaviour, or something that existing frameworks all have to prevent on their own? (By calling suppressOffers() on reregistered(), I would guess ?..)
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70894/#review215983
-----------------------------------------------------------
On June 19, 2019, 6:26 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70894/
> -----------------------------------------------------------
>
> (Updated June 19, 2019, 6:26 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.
>
>
> Diffs
> -----
>
> include/mesos/scheduler.hpp 8c6774885ceb3b0644c32842a17fba39e2c3e472
> src/sched/sched.cpp e6cc534264e3ff4edaa703a660afb2f896388802
>
>
> Diff: https://reviews.apache.org/r/70894/diff/1/
>
>
> 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
>
>
Re: Review Request 70894: Provided ability to change suppressed roles
via V0 updateFramework().
Posted by Andrei Sekretenko <as...@mesosphere.io>.
> On June 19, 2019, 10:54 p.m., Benjamin Mahler wrote:
> > Hm.. there's a relationship between the 'suppressedRoles' list and the reviveOffers() / suppressOffers() calls.
> >
> > For example, if I call reviveOffers(), I would then expect that 'suppressedRoles' will be cleared and won't be sent if a re-registration happens under the covers due to a disconnection, e.g.
> >
> > ```
> > updateFramework(f, ["role1", "role2"]); // UPDATE_FRAMEWORK is sent
> > reviveOffers(); // REVIVE is sent, logically the master moves the suppressed roles from ["role1", "role2"] to []!
> > // disconnection and re-registration happens
> > // at this point, the driver should send [] as suppressed, since the scheduler unsuppressed all roles
> > // (but it sends ["role1", "role2"] in this patch)
> > ```
> >
> > Similarly for suppressOffers() where all of the roles become suppressed.
> >
> > If we add role(s) arguments to reviveOffers() / suppressOffers() then they would be removing items from the suppressed set rather than clearing and assigning it entirely.
>
> Andrei Sekretenko wrote:
> Good point.
>
> Now I'm wondering if the already existing behaviour of suppressOffers() + reconnection is valid.
> Consder a similar chain of events:
> - framework starts the driver
> - framework calls suppressOffers()
> - due to some reason, driver reconnects (to the same master or to the new one), all roles are active again
>
> Is this a desired behaviour, or something that existing frameworks all have to prevent on their own? (By calling suppressOffers() on reregistered(), I would guess ?..)
I've added storing the suppressed roles (as a set - this will be needed in the next patches) and clearing them in `reviveOffers()`.
`suppressOffers()` will be addressed in a separate patch, as it might affect behavior of existing frameworks and we might have to revert that patch and deprecate `suppressOffers()` instead.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70894/#review215983
-----------------------------------------------------------
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
>
>
Re: Review Request 70894: Provided ability to change suppressed roles
via V0 updateFramework().
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70894/#review215983
-----------------------------------------------------------
Hm.. there's a relationship between the 'suppressedRoles' list and the reviveOffers() / suppressOffers() calls.
For example, if I call reviveOffers(), I would then expect that 'suppressedRoles' will be cleared and won't be sent if a re-registration happens under the covers due to a disconnection, e.g.
```
updateFramework(f, ["role1", "role2"]); // UPDATE_FRAMEWORK is sent
reviveOffers(); // REVIVE is sent, logically the master moves the suppressed roles from ["role1", "role2"] to []!
// disconnection and re-registration happens
// at this point, the driver should send [] as suppressed, since the scheduler unsuppressed all roles
// (but it sends ["role1", "role2"] in this patch)
```
Similarly for suppressOffers() where all of the roles become suppressed.
If we add role(s) arguments to reviveOffers() / suppressOffers() then they would be removing items from the suppressed set rather than clearing and assigning it entirely.
src/sched/sched.cpp
Lines 1697 (patched)
<https://reviews.apache.org/r/70894/#comment302906>
why bother storing it as a repeated field instead of a vector? we still have to copy it
- Benjamin Mahler
On June 19, 2019, 6:26 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70894/
> -----------------------------------------------------------
>
> (Updated June 19, 2019, 6:26 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.
>
>
> Diffs
> -----
>
> include/mesos/scheduler.hpp 8c6774885ceb3b0644c32842a17fba39e2c3e472
> src/sched/sched.cpp e6cc534264e3ff4edaa703a660afb2f896388802
>
>
> Diff: https://reviews.apache.org/r/70894/diff/1/
>
>
> 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
>
>