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/25 15:57:50 UTC
Review Request 70941: Added to the scheduler driver a method to
revive a subset of roles.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70941/
-----------------------------------------------------------
Review request for mesos and Benjamin Mahler.
Bugs: MESOS-9849
https://issues.apache.org/jira/browse/MESOS-9849
Repository: mesos
Description
-------
This patch adds to the scheduler driver a 'reviveOffers(roles)' method,
which sends the REVIVE call for these roles and removes them from the
suppressed roles set.
Diffs
-----
include/mesos/scheduler.hpp 8c6774885ceb3b0644c32842a17fba39e2c3e472
src/sched/sched.cpp e6cc534264e3ff4edaa703a660afb2f896388802
Diff: https://reviews.apache.org/r/70941/diff/1/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 70941: Added to the scheduler driver a method to
revive a subset of roles.
Posted by Benjamin Mahler <bm...@apache.org>.
> On June 25, 2019, 7:36 p.m., Benjamin Mahler wrote:
> > src/sched/sched.cpp
> > Line 1439 (original), 1440 (patched)
> > <https://reviews.apache.org/r/70941/diff/1/?file=2152139#file2152139line1440>
> >
> > I don't think we need the option here. In the protobuf, we treat the empty list of Revive.roles to mean all roles, we can do that here too to be consistent.
>
> Andrei Sekretenko wrote:
> Good point. I didn't take consistency with the protobuf (and thus V1 API) into consideration.
>
> Now an empty list means "revive all roles", not "revive nothing".
Sorry for the run-around, thinking about it again, it’s more surprising from a C++ perspective to pass an empty list and everything gets revived, whereas in the protobuf it’s a bit more clear (if you don’t set the new field you get the “old” behavior of the Revive message)
However, I guess the original behavior in the patch was wrong, since it would pass an empty list through to protobuf and that would revive all roles rather than none.
Probably the right behavior here is to have:
```
// revive all roles
Status reviveOffers() override;
// revive provided roles (therefore empty == no-op)
Status reviveOffers(const std::vector<std::string>& roles) override;
```
this means that the implementation needs to special case the empty case
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70941/#review216129
-----------------------------------------------------------
On July 1, 2019, 1:46 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70941/
> -----------------------------------------------------------
>
> (Updated July 1, 2019, 1:46 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9849
> https://issues.apache.org/jira/browse/MESOS-9849
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds to the scheduler driver a 'reviveOffers(roles)' method,
> which sends the REVIVE call for these roles and removes them from the
> suppressed roles set.
>
>
> Diffs
> -----
>
> include/mesos/scheduler.hpp 8c6774885ceb3b0644c32842a17fba39e2c3e472
> src/sched/sched.cpp e6cc534264e3ff4edaa703a660afb2f896388802
>
>
> Diff: https://reviews.apache.org/r/70941/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70941: Added to the scheduler driver a method to
revive a subset of roles.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
> On June 25, 2019, 7:36 p.m., Benjamin Mahler wrote:
> > src/sched/sched.cpp
> > Line 1439 (original), 1440 (patched)
> > <https://reviews.apache.org/r/70941/diff/1/?file=2152139#file2152139line1440>
> >
> > I don't think we need the option here. In the protobuf, we treat the empty list of Revive.roles to mean all roles, we can do that here too to be consistent.
Good point. I didn't take consistency with the protobuf (and thus V1 API) into consideration.
Now an empty list means "revive all roles", not "revive nothing".
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70941/#review216129
-----------------------------------------------------------
On June 25, 2019, 3:57 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70941/
> -----------------------------------------------------------
>
> (Updated June 25, 2019, 3:57 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9849
> https://issues.apache.org/jira/browse/MESOS-9849
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds to the scheduler driver a 'reviveOffers(roles)' method,
> which sends the REVIVE call for these roles and removes them from the
> suppressed roles set.
>
>
> Diffs
> -----
>
> include/mesos/scheduler.hpp 8c6774885ceb3b0644c32842a17fba39e2c3e472
> src/sched/sched.cpp e6cc534264e3ff4edaa703a660afb2f896388802
>
>
> Diff: https://reviews.apache.org/r/70941/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70941: Added to the scheduler driver a method to
revive a subset of roles.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70941/#review216129
-----------------------------------------------------------
Looks good, just some minor suggestions below
src/sched/sched.cpp
Line 1439 (original), 1440 (patched)
<https://reviews.apache.org/r/70941/#comment303141>
I don't think we need the option here. In the protobuf, we treat the empty list of Revive.roles to mean all roles, we can do that here too to be consistent.
src/sched/sched.cpp
Lines 1445 (patched)
<https://reviews.apache.org/r/70941/#comment303140>
`*roles`
src/sched/sched.cpp
Line 2281 (original), 2293 (patched)
<https://reviews.apache.org/r/70941/#comment303142>
Per comment above, this can pass in the empty list to be more consistent with the protobuf api.
src/sched/sched.cpp
Lines 2300 (patched)
<https://reviews.apache.org/r/70941/#comment303143>
`s/roles_/roles/`
- Benjamin Mahler
On June 25, 2019, 3:57 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70941/
> -----------------------------------------------------------
>
> (Updated June 25, 2019, 3:57 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9849
> https://issues.apache.org/jira/browse/MESOS-9849
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds to the scheduler driver a 'reviveOffers(roles)' method,
> which sends the REVIVE call for these roles and removes them from the
> suppressed roles set.
>
>
> Diffs
> -----
>
> include/mesos/scheduler.hpp 8c6774885ceb3b0644c32842a17fba39e2c3e472
> src/sched/sched.cpp e6cc534264e3ff4edaa703a660afb2f896388802
>
>
> Diff: https://reviews.apache.org/r/70941/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70941: Added to the scheduler driver a method to
revive a subset of roles.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70941/#review216365
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Mahler
On July 3, 2019, 3:52 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70941/
> -----------------------------------------------------------
>
> (Updated July 3, 2019, 3:52 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9849
> https://issues.apache.org/jira/browse/MESOS-9849
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds to the scheduler driver a 'reviveOffers(roles)' method,
> which sends the REVIVE call for these roles and removes them from the
> suppressed roles set.
>
>
> Diffs
> -----
>
> include/mesos/scheduler.hpp 0a09d5505ed2f9f65897ecce44cffa75db624ef4
> src/sched/sched.cpp 6b02ac071c9773411954636730c953cca0044516
>
>
> Diff: https://reviews.apache.org/r/70941/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70941: Added to the scheduler driver a method to
revive a subset of roles.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70941/
-----------------------------------------------------------
(Updated July 3, 2019, 3:52 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Made reviving an empty vector of roles a no-op.
Bugs: MESOS-9849
https://issues.apache.org/jira/browse/MESOS-9849
Repository: mesos
Description
-------
This patch adds to the scheduler driver a 'reviveOffers(roles)' method,
which sends the REVIVE call for these roles and removes them from the
suppressed roles set.
Diffs (updated)
-----
include/mesos/scheduler.hpp 0a09d5505ed2f9f65897ecce44cffa75db624ef4
src/sched/sched.cpp 6b02ac071c9773411954636730c953cca0044516
Diff: https://reviews.apache.org/r/70941/diff/3/
Changes: https://reviews.apache.org/r/70941/diff/2-3/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 70941: Added to the scheduler driver a method to
revive a subset of roles.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70941/
-----------------------------------------------------------
(Updated July 1, 2019, 1:46 p.m.)
Review request for mesos and Benjamin Mahler.
Bugs: MESOS-9849
https://issues.apache.org/jira/browse/MESOS-9849
Repository: mesos
Description
-------
This patch adds to the scheduler driver a 'reviveOffers(roles)' method,
which sends the REVIVE call for these roles and removes them from the
suppressed roles set.
Diffs (updated)
-----
include/mesos/scheduler.hpp 8c6774885ceb3b0644c32842a17fba39e2c3e472
src/sched/sched.cpp e6cc534264e3ff4edaa703a660afb2f896388802
Diff: https://reviews.apache.org/r/70941/diff/2/
Changes: https://reviews.apache.org/r/70941/diff/1-2/
Testing
-------
Thanks,
Andrei Sekretenko