Re: Review Request 34571: Pushed QoS Correction stub message to mesos.proto

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34571/#review84829
---


Patch looks great!

Reviews applied: [34571]

All tests passed.

- Mesos ReviewBot


On May 21, 2015, 11:02 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34571/
> ---
> 
> (Updated May 21, 2015, 11:02 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This part of proto describes a QoS corrections message which includes 
> corrections for particular executors or tasks.
> It is a generic message between QoS Controller and slave.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto a66888916dc90e476c8bb20e67e3f6b08c47fb99 
> 
> Diff: https://reviews.apache.org/r/34571/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 34571: Pushed QoS Correction stub message to mesos.proto

[Niklas Nielsen]

> On May 21, 2015, 4:24 p.m., Jie Yu wrote:
> > include/mesos/mesos.proto, line 1189
> > 
> >
> > This is a protocol between slave and the QoSController, both of which 
> > are internal to Mesos. So we probably should move this message definition 
> > to src/messages/messages.proto.
> 
> Niklas Nielsen wrote:
> We need this to be public to make QoS Controller modules :)
> 
> Jie Yu wrote:
> ah. Then how about creating a new proto file
> 
> include/mesos/slave/oversubscription.proto

I like it!


- Niklas


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34571/#review84817
---


On May 21, 2015, 4:02 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34571/
> ---
> 
> (Updated May 21, 2015, 4:02 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This part of proto describes a QoS corrections message which includes 
> corrections for particular executors or tasks.
> It is a generic message between QoS Controller and slave.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto a66888916dc90e476c8bb20e67e3f6b08c47fb99 
> 
> Diff: https://reviews.apache.org/r/34571/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 34571: Pushed QoS Correction stub message to mesos.proto

[Jie Yu]

> On May 21, 2015, 11:24 p.m., Jie Yu wrote:
> > include/mesos/mesos.proto, line 1189
> > 
> >
> > This is a protocol between slave and the QoSController, both of which 
> > are internal to Mesos. So we probably should move this message definition 
> > to src/messages/messages.proto.
> 
> Niklas Nielsen wrote:
> We need this to be public to make QoS Controller modules :)

ah. Then how about creating a new proto file

include/mesos/slave/oversubscription.proto


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34571/#review84817
---


On May 21, 2015, 11:02 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34571/
> ---
> 
> (Updated May 21, 2015, 11:02 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This part of proto describes a QoS corrections message which includes 
> corrections for particular executors or tasks.
> It is a generic message between QoS Controller and slave.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto a66888916dc90e476c8bb20e67e3f6b08c47fb99 
> 
> Diff: https://reviews.apache.org/r/34571/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 34571: Pushed QoS Correction stub message to mesos.proto

[Niklas Nielsen]

> On May 21, 2015, 4:24 p.m., Jie Yu wrote:
> > include/mesos/mesos.proto, line 1189
> > 
> >
> > This is a protocol between slave and the QoSController, both of which 
> > are internal to Mesos. So we probably should move this message definition 
> > to src/messages/messages.proto.

We need this to be public to make QoS Controller modules :)


> On May 21, 2015, 4:24 p.m., Jie Yu wrote:
> > include/mesos/mesos.proto, lines 1189-1211
> > 
> >
> > It reads weired when one wants to create a QoS correction (too many 
> > words 'correction'):
> > 
> > ```
> > QoSCorrections::Correction correction;
> > ```
> > 
> > How about we just define the `QoSCorrection` message and create a 
> > wrapper class `QoSCorrections` in C++ (similar to what we did for 
> > `Resource`)?
> 
> Bartek Plotka wrote:
> Yeah that's the same story as with the Resource - we can do that.

SGTM - or just return a list of corrections. Don't have a strong opinion on 
which one.


- Niklas


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34571/#review84817
---


On May 21, 2015, 4:02 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34571/
> ---
> 
> (Updated May 21, 2015, 4:02 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This part of proto describes a QoS corrections message which includes 
> corrections for particular executors or tasks.
> It is a generic message between QoS Controller and slave.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto a66888916dc90e476c8bb20e67e3f6b08c47fb99 
> 
> Diff: https://reviews.apache.org/r/34571/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 34571: Pushed QoS Correction stub message to mesos.proto

[Bartek Plotka]

> On May 21, 2015, 11:24 p.m., Jie Yu wrote:
> > include/mesos/mesos.proto, lines 1189-1211
> > 
> >
> > It reads weired when one wants to create a QoS correction (too many 
> > words 'correction'):
> > 
> > ```
> > QoSCorrections::Correction correction;
> > ```
> > 
> > How about we just define the `QoSCorrection` message and create a 
> > wrapper class `QoSCorrections` in C++ (similar to what we did for 
> > `Resource`)?

Yeah that's the same story as with the Resource - we can do that.


- Bartek


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34571/#review84817
---


On May 21, 2015, 11:02 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34571/
> ---
> 
> (Updated May 21, 2015, 11:02 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This part of proto describes a QoS corrections message which includes 
> corrections for particular executors or tasks.
> It is a generic message between QoS Controller and slave.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto a66888916dc90e476c8bb20e67e3f6b08c47fb99 
> 
> Diff: https://reviews.apache.org/r/34571/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 34571: Pushed QoS Correction stub message to mesos.proto

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34571/#review84817
---



include/mesos/mesos.proto


This is a protocol between slave and the QoSController, both of which are 
internal to Mesos. So we probably should move this message definition to 
src/messages/messages.proto.



include/mesos/mesos.proto


It reads weired when one wants to create a QoS correction (too many words 
'correction'):

```
QoSCorrections::Correction correction;
```

How about we just define the `QoSCorrection` message and create a wrapper 
class `QoSCorrections` in C++ (similar to what we did for `Resource`)?



include/mesos/mesos.proto


I liked Niklas's suggestion. How about following the style used by 
Offer.Operation. Also, Let's try not to introduce those fields that are 
unneeded right now.

```
message QoSCorrection {
  enum Type {
KILL = 1;
  }
  
  message Kill {
required ContainerID container_id = 1;
  }
  
  required Type type = 1;
  optional Kill kill = 2;
}
```


- Jie Yu


On May 21, 2015, 11:02 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34571/
> ---
> 
> (Updated May 21, 2015, 11:02 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This part of proto describes a QoS corrections message which includes 
> corrections for particular executors or tasks.
> It is a generic message between QoS Controller and slave.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto a66888916dc90e476c8bb20e67e3f6b08c47fb99 
> 
> Diff: https://reviews.apache.org/r/34571/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 34571: Pushed QoS Correction stub message to mesos.proto

[Szymon Konefal]

> On May 21, 2015, 11:08 p.m., Niklas Nielsen wrote:
> > A higher level question is whether we should have touples of actions.
> > 
> > For example:
> > 
> > message KillAction {
> >   optional ExecutorID executor_id = X;
> > }
> > 
> > message ResizeAction {
> >   optional TaskID task_id = X;
> >   optional Resources resources = Y;
> > }
> > 
> > ...

Yes, the resizing action of custom isolators could be tricky to formalize.
We have two choices:
1) Make an union of possible actions with bodies
2) Use labels to carry metadata

I would go for the first choice. If someone would like to resize his custom 
iso, one should need to extend the ResiseAction message.


- Szymon


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34571/#review84814
---


On May 21, 2015, 11:02 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34571/
> ---
> 
> (Updated May 21, 2015, 11:02 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This part of proto describes a QoS corrections message which includes 
> corrections for particular executors or tasks.
> It is a generic message between QoS Controller and slave.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto a66888916dc90e476c8bb20e67e3f6b08c47fb99 
> 
> Diff: https://reviews.apache.org/r/34571/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 34571: Pushed QoS Correction stub message to mesos.proto

[Niklas Nielsen]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34571/#review84814
---


A higher level question is whether we should have touples of actions.

For example:

message KillAction {
  optional ExecutorID executor_id = X;
}

message ResizeAction {
  optional TaskID task_id = X;
  optional Resources resources = Y;
}

...


include/mesos/mesos.proto


Kill this newline :)



include/mesos/mesos.proto


And this one.


- Niklas Nielsen


On May 21, 2015, 4:02 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34571/
> ---
> 
> (Updated May 21, 2015, 4:02 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This part of proto describes a QoS corrections message which includes 
> corrections for particular executors or tasks.
> It is a generic message between QoS Controller and slave.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto a66888916dc90e476c8bb20e67e3f6b08c47fb99 
> 
> Diff: https://reviews.apache.org/r/34571/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 34571: Pushed QoS Correction stub message to mesos.proto

[Bartek Plotka]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34571/
---

(Updated May 21, 2015, 4:02 p.m.)


Review request for mesos, Jie Yu, Niklas Nielsen, and Vinod Kone.


Bugs: MESOS-2760
https://issues.apache.org/jira/browse/MESOS-2760


Repository: mesos


Description (updated)
---

This part of proto describes a QoS corrections message which includes 
corrections for particular executors or tasks.
It is a generic message between QoS Controller and slave.


Diffs
-

  include/mesos/mesos.proto a66888916dc90e476c8bb20e67e3f6b08c47fb99 

Diff: https://reviews.apache.org/r/34571/diff/


Testing
---

make check


Thanks,

Bartek Plotka

Review Request 34571: Pushed QoS Correction stub message to mesos.proto

[Bartek Plotka]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34571/
---

Review request for mesos, Jie Yu, Niklas Nielsen, and Vinod Kone.


Bugs: MESOS-2760
https://issues.apache.org/jira/browse/MESOS-2760


Repository: mesos


Description
---

Pushed QoS Correction stub message to mesos.proto.

This part of proto describies QoS corrections message which includes 
corrections for particular executor or task.
It's generic message between QoS Controller and slave.


Diffs
-

  include/mesos/mesos.proto a66888916dc90e476c8bb20e67e3f6b08c47fb99 

Diff: https://reviews.apache.org/r/34571/diff/


Testing
---

make check


Thanks,

Bartek Plotka