You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Anindya Sinha <an...@apple.com> on 2017/04/15 06:40:25 UTC
Re: Review Request 57815: Added `suppress` field in SUBSCRIBE.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57815/
-----------------------------------------------------------
(Updated April 15, 2017, 6:40 a.m.)
Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
Changes
-------
Updated based on comments in this review chain.
Summary (updated)
-----------------
Added `suppress` field in SUBSCRIBE.
Bugs: MESOS-7015
https://issues.apache.org/jira/browse/MESOS-7015
Repository: mesos
Description (updated)
-------
If set, offers are suppressed on registration for this framework.
By default, offers shall not be suppressed on successful
registration with the master.
Diffs (updated)
-----
include/mesos/scheduler/scheduler.proto cee0a175fcdcbb8b56d41a5b4b6fd965afd822bd
include/mesos/v1/scheduler/scheduler.proto 00a54eceb435de8e8fea3ccab285f39ed13c6fee
Diff: https://reviews.apache.org/r/57815/diff/3/
Changes: https://reviews.apache.org/r/57815/diff/2-3/
Testing
-------
All tests passed.
Thanks,
Anindya Sinha
Re: Review Request 57815: Added `deactivated_roles` field in
`FrameworkInfo`.
Posted by Anindya Sinha <an...@apple.com>.
> On April 17, 2017, 6:28 p.m., Vinod Kone wrote:
> > include/mesos/scheduler/scheduler.proto
> > Lines 249-250 (original), 249-250 (patched)
> > <https://reviews.apache.org/r/57815/diff/1-3/?file=1670985#file1670985line249>
> >
> > Hmm, I was hoping that we could re-use the `Call::Suppress` message here instead of re-defining it. That way any changes to suppress would be in sync.
>
> Anindya Sinha wrote:
> Currently, `Call::Suppress` is:
> ```
> message Suppress {
> optional string role = 1;
> }
> ```
>
> So, the `SUPPRESS` call currently allows suppressing offers for a framework on a per-role basis (or all roles if `role` is not set). Since we now support multiple roles for a frameework, and if we want to suppress offers for a subset of roles at the time of `SUBSCRIBE`, using `Call::Suppress` would not satisfy that use case. It would only satisfy suppressing for all roles (i.e. `Suppress::role` is not set) or for a specific role.
>
> Is there any plans of extending `SUPPRESS` to suppress offers for a subset of roles (>0, and < all roles) in a single call? Do you think we should consider that also?
>
> One such approach could be to have the following:
> ```
> message Suppress {
> repeated string roles = 1;
> }
>
> optional Suppress suppress;
> ```
>
> If suppress is not set, then we do not suppress for any roles. Otherwise, we suppress for the set of roles specified in `suppress.roles`. If `suppress` is set but `suppress.roles` is empty, we suppress for all roles of the framework.
>
> Thoughts?
So based on discussion in the Slack channel, this modified review chain does the following:
1. Add `repeated string deactivated_roles` to `FrameworkInfo` which represents a subset of roles which are deactivated. Offers pertaining to the deactivated roles shall not be sent out.
2. `SUPPRESS` and `REVIVE` calls will toggle the (de)activation mode of the roles.
3. Allocator's `activateFramework()` call will activate all roles of the framework which are not in `FrameworkInfo::deactivated+roles`. Similarly, in `deactivateFramework()` call will deactivate all roles that are not deactivated already.
- Anindya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57815/#review172115
-----------------------------------------------------------
On May 5, 2017, 12:34 a.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57815/
> -----------------------------------------------------------
>
> (Updated May 5, 2017, 12:34 a.m.)
>
>
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-7015
> https://issues.apache.org/jira/browse/MESOS-7015
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This field is a subset of roles the framework registered as for which
> the framework does not want any resources offere to.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 1935f47a52840f6b395ecb2d28296666551fa691
> include/mesos/v1/mesos.proto c7f0bec5c96f2f41344d4261d0696f9fe0421db7
>
>
> Diff: https://reviews.apache.org/r/57815/diff/4/
>
>
> Testing
> -------
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 57815: Added `suppress` field in SUBSCRIBE.
Posted by Anindya Sinha <an...@apple.com>.
> On April 17, 2017, 6:28 p.m., Vinod Kone wrote:
> > include/mesos/scheduler/scheduler.proto
> > Lines 249-250 (original), 249-250 (patched)
> > <https://reviews.apache.org/r/57815/diff/1-3/?file=1670985#file1670985line249>
> >
> > Hmm, I was hoping that we could re-use the `Call::Suppress` message here instead of re-defining it. That way any changes to suppress would be in sync.
Currently, `Call::Suppress` is:
```
message Suppress {
optional string role = 1;
}
```
So, the `SUPPRESS` call currently allows suppressing offers for a framework on a per-role basis (or all roles if `role` is not set). Since we now support multiple roles for a frameework, and if we want to suppress offers for a subset of roles at the time of `SUBSCRIBE`, using `Call::Suppress` would not satisfy that use case. It would only satisfy suppressing for all roles (i.e. `Suppress::role` is not set) or for a specific role.
Is there any plans of extending `SUPPRESS` to suppress offers for a subset of roles (>0, and < all roles) in a single call? Do you think we should consider that also?
One such approach could be to have the following:
```
message Suppress {
repeated string roles = 1;
}
optional Suppress suppress;
```
If suppress is not set, then we do not suppress for any roles. Otherwise, we suppress for the set of roles specified in `suppress.roles`. If `suppress` is set but `suppress.roles` is empty, we suppress for all roles of the framework.
Thoughts?
- Anindya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57815/#review172115
-----------------------------------------------------------
On April 15, 2017, 6:40 a.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57815/
> -----------------------------------------------------------
>
> (Updated April 15, 2017, 6:40 a.m.)
>
>
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-7015
> https://issues.apache.org/jira/browse/MESOS-7015
>
>
> Repository: mesos
>
>
> Description
> -------
>
> If set, offers are suppressed on registration for this framework.
> By default, offers shall not be suppressed on successful
> registration with the master.
>
>
> Diffs
> -----
>
> include/mesos/scheduler/scheduler.proto cee0a175fcdcbb8b56d41a5b4b6fd965afd822bd
> include/mesos/v1/scheduler/scheduler.proto 00a54eceb435de8e8fea3ccab285f39ed13c6fee
>
>
> Diff: https://reviews.apache.org/r/57815/diff/3/
>
>
> Testing
> -------
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 57815: Added `suppressed_roles` field in
`SUBSCRIBE`.
Posted by Anindya Sinha <an...@apple.com>.
> On April 17, 2017, 6:28 p.m., Vinod Kone wrote:
> > include/mesos/scheduler/scheduler.proto
> > Lines 249-250 (original), 249-250 (patched)
> > <https://reviews.apache.org/r/57815/diff/1-3/?file=1670985#file1670985line249>
> >
> > Hmm, I was hoping that we could re-use the `Call::Suppress` message here instead of re-defining it. That way any changes to suppress would be in sync.
>
> Anindya Sinha wrote:
> Currently, `Call::Suppress` is:
> ```
> message Suppress {
> optional string role = 1;
> }
> ```
>
> So, the `SUPPRESS` call currently allows suppressing offers for a framework on a per-role basis (or all roles if `role` is not set). Since we now support multiple roles for a frameework, and if we want to suppress offers for a subset of roles at the time of `SUBSCRIBE`, using `Call::Suppress` would not satisfy that use case. It would only satisfy suppressing for all roles (i.e. `Suppress::role` is not set) or for a specific role.
>
> Is there any plans of extending `SUPPRESS` to suppress offers for a subset of roles (>0, and < all roles) in a single call? Do you think we should consider that also?
>
> One such approach could be to have the following:
> ```
> message Suppress {
> repeated string roles = 1;
> }
>
> optional Suppress suppress;
> ```
>
> If suppress is not set, then we do not suppress for any roles. Otherwise, we suppress for the set of roles specified in `suppress.roles`. If `suppress` is set but `suppress.roles` is empty, we suppress for all roles of the framework.
>
> Thoughts?
>
> Anindya Sinha wrote:
> So based on discussion in the Slack channel, this modified review chain does the following:
>
> 1. Add `repeated string deactivated_roles` to `FrameworkInfo` which represents a subset of roles which are deactivated. Offers pertaining to the deactivated roles shall not be sent out.
> 2. `SUPPRESS` and `REVIVE` calls will toggle the (de)activation mode of the roles.
> 3. Allocator's `activateFramework()` call will activate all roles of the framework which are not in `FrameworkInfo::deactivated+roles`. Similarly, in `deactivateFramework()` call will deactivate all roles that are not deactivated already.
Based on discussion on Slack, this is what was decided:
1. Add `repeated string suppressed_roles` to `Call::Subscribe` which represents a subset of roles which are suppressed. Offers pertaining to the suppressed roles shall not be sent out.
2. `SUPPRESS` and `REVIVE` calls will toggle the (de)activation mode of the roles.
3. Allocator's `activateFramework()` call will activate all roles of the framework which are not in suppressed_roles. Similarly, in `deactivateFramework()` call will deactivate all roles that are not suppressed already.
Note that this functionality shall not be added for scheduler driver in this review chain (for MESOS-7015). I opened https://issues.apache.org/jira/browse/MESOS-7526 to address that.
- Anindya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57815/#review172115
-----------------------------------------------------------
On May 19, 2017, 6:26 p.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57815/
> -----------------------------------------------------------
>
> (Updated May 19, 2017, 6:26 p.m.)
>
>
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-7015
> https://issues.apache.org/jira/browse/MESOS-7015
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This field is a subset of roles the framework registered as for which
> the framework does not want any resources offere to.
>
>
> Diffs
> -----
>
> include/mesos/scheduler/scheduler.proto f83b2ce7e88e83abc4e523b06333c448a3dcfd01
> include/mesos/v1/scheduler/scheduler.proto d923cb9dc4205e4b601feebba84e3b30091ea3e0
>
>
> Diff: https://reviews.apache.org/r/57815/diff/6/
>
>
> Testing
> -------
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 57815: Added `suppress` field in SUBSCRIBE.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57815/#review172115
-----------------------------------------------------------
include/mesos/scheduler/scheduler.proto
Lines 249-250 (original), 249-250 (patched)
<https://reviews.apache.org/r/57815/#comment245193>
Hmm, I was hoping that we could re-use the `Call::Suppress` message here instead of re-defining it. That way any changes to suppress would be in sync.
- Vinod Kone
On April 15, 2017, 6:40 a.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57815/
> -----------------------------------------------------------
>
> (Updated April 15, 2017, 6:40 a.m.)
>
>
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-7015
> https://issues.apache.org/jira/browse/MESOS-7015
>
>
> Repository: mesos
>
>
> Description
> -------
>
> If set, offers are suppressed on registration for this framework.
> By default, offers shall not be suppressed on successful
> registration with the master.
>
>
> Diffs
> -----
>
> include/mesos/scheduler/scheduler.proto cee0a175fcdcbb8b56d41a5b4b6fd965afd822bd
> include/mesos/v1/scheduler/scheduler.proto 00a54eceb435de8e8fea3ccab285f39ed13c6fee
>
>
> Diff: https://reviews.apache.org/r/57815/diff/3/
>
>
> Testing
> -------
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 57815: Added `suppressed_roles` field in
`SUBSCRIBE`.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57815/#review175838
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On May 19, 2017, 6:26 p.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57815/
> -----------------------------------------------------------
>
> (Updated May 19, 2017, 6:26 p.m.)
>
>
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-7015
> https://issues.apache.org/jira/browse/MESOS-7015
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This field is a subset of roles the framework registered as for which
> the framework does not want any resources offere to.
>
>
> Diffs
> -----
>
> include/mesos/scheduler/scheduler.proto f83b2ce7e88e83abc4e523b06333c448a3dcfd01
> include/mesos/v1/scheduler/scheduler.proto d923cb9dc4205e4b601feebba84e3b30091ea3e0
>
>
> Diff: https://reviews.apache.org/r/57815/diff/6/
>
>
> Testing
> -------
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 57815: Added `suppressed_roles` field in
`SUBSCRIBE`.
Posted by Anindya Sinha <an...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57815/
-----------------------------------------------------------
(Updated May 19, 2017, 6:26 p.m.)
Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
Changes
-------
Updates based on agreed approach.
Summary (updated)
-----------------
Added `suppressed_roles` field in `SUBSCRIBE`.
Bugs: MESOS-7015
https://issues.apache.org/jira/browse/MESOS-7015
Repository: mesos
Description
-------
This field is a subset of roles the framework registered as for which
the framework does not want any resources offere to.
Diffs (updated)
-----
include/mesos/scheduler/scheduler.proto f83b2ce7e88e83abc4e523b06333c448a3dcfd01
include/mesos/v1/scheduler/scheduler.proto d923cb9dc4205e4b601feebba84e3b30091ea3e0
Diff: https://reviews.apache.org/r/57815/diff/6/
Changes: https://reviews.apache.org/r/57815/diff/5-6/
Testing
-------
All tests passed.
Thanks,
Anindya Sinha
Re: Review Request 57815: Added `deactivated_roles` field in
`FrameworkInfo`.
Posted by Anindya Sinha <an...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57815/
-----------------------------------------------------------
(Updated May 6, 2017, 10:30 p.m.)
Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
Changes
-------
Rebased.
Bugs: MESOS-7015
https://issues.apache.org/jira/browse/MESOS-7015
Repository: mesos
Description
-------
This field is a subset of roles the framework registered as for which
the framework does not want any resources offere to.
Diffs (updated)
-----
include/mesos/mesos.proto 1935f47a52840f6b395ecb2d28296666551fa691
include/mesos/v1/mesos.proto c7f0bec5c96f2f41344d4261d0696f9fe0421db7
Diff: https://reviews.apache.org/r/57815/diff/5/
Changes: https://reviews.apache.org/r/57815/diff/4-5/
Testing
-------
All tests passed.
Thanks,
Anindya Sinha
Re: Review Request 57815: Added `deactivated_roles` field in
`FrameworkInfo`.
Posted by Anindya Sinha <an...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57815/
-----------------------------------------------------------
(Updated May 5, 2017, 12:34 a.m.)
Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
Changes
-------
Addressed review comments based on discussion on Slack.
Summary (updated)
-----------------
Added `deactivated_roles` field in `FrameworkInfo`.
Bugs: MESOS-7015
https://issues.apache.org/jira/browse/MESOS-7015
Repository: mesos
Description (updated)
-------
This field is a subset of roles the framework registered as for which
the framework does not want any resources offere to.
Diffs (updated)
-----
include/mesos/mesos.proto 1935f47a52840f6b395ecb2d28296666551fa691
include/mesos/v1/mesos.proto c7f0bec5c96f2f41344d4261d0696f9fe0421db7
Diff: https://reviews.apache.org/r/57815/diff/4/
Changes: https://reviews.apache.org/r/57815/diff/3-4/
Testing
-------
All tests passed.
Thanks,
Anindya Sinha