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 70941: Added to the scheduler driver a method to revive a subset of roles.
Date Tue, 02 Jul 2019 04:10:07 GMT


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


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