mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 70894: Provided ability to change suppressed roles via V0 updateFramework().
Date Tue, 02 Jul 2019 03:26:15 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message