You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Bartek Plotka <bw...@gmail.com> on 2015/05/22 04:30:02 UTC
Review Request 34581: Added oversubscription.proto for QoS Controller
and slave communication
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/
-----------------------------------------------------------
Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
Bugs: MESOS-2760
https://issues.apache.org/jira/browse/MESOS-2760
Repository: mesos
Description
-------
This proto describes a QoS correction message for particular executor or task.
It is a generic message between QoS Controller and slave.
Additionaly, updated Makefile to include this proto during compilation.
This request updates the https://reviews.apache.org/r/34571/
Diffs
-----
include/mesos/slave/oversubscription.proto PRE-CREATION
src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
Diff: https://reviews.apache.org/r/34581/diff/
Testing
-------
* make check
* run mesos:
1) build
2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
Thanks,
Bartek Plotka
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review84944
-----------------------------------------------------------
Looks good! A few nits and let's get it in
include/mesos/slave/oversubscription.proto
<https://reviews.apache.org/r/34581/#comment136407>
What do you think about moving this comment into the enum?
include/mesos/slave/oversubscription.proto
<https://reviews.apache.org/r/34581/#comment136406>
Mind putting a space between comment and 'Kill'?
Also, we end comments with periods :)
src/Makefile.am
<https://reviews.apache.org/r/34581/#comment136396>
The alignment is a bit off - set tabstop to 8 for Makefiles :)
Here and above
src/Makefile.am
<https://reviews.apache.org/r/34581/#comment136397>
Alignment is off here too.
- Niklas Nielsen
On May 21, 2015, 7:31 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 21, 2015, 7:31 p.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review84858
-----------------------------------------------------------
Patch looks great!
Reviews applied: [34581]
All tests passed.
- Mesos ReviewBot
On May 22, 2015, 2:31 a.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 22, 2015, 2:31 a.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Bartek Plotka <bw...@gmail.com>.
> On May 22, 2015, 8:50 p.m., Marco Massenzio wrote:
> > include/mesos/slave/oversubscription.proto, line 34
> > <https://reviews.apache.org/r/34581/diff/2/?file=969904#file969904line34>
> >
> > please consider calling this `QosCorrectiveAction`
> > (we require CamelCase for our types, in any event; so this would have to be `QosCorrection`)
> >
> > I'm also not wild about the `QoS` moniker - I'd like this to be a more generic `CorrectiveAction` message, but happy to go with whatever else others suggest.
>
> Jie Yu wrote:
> I prefer QoSCorrection since QoS is an abbreivation. THis is similar to SlaveID and we don't callid SlaveId :)
>
> Niklas Nielsen wrote:
> +1
>
> Marco Massenzio wrote:
> For the record, SlaveID is a violation of the Google Style guide we purportedly follow - it's probably too late to fix now, but we should avoid to perpetuate the mistake.
>
> The rationale of the rule is that it does not leave room for guessing the uppercase/lowercase: it's strictly CamelCase (so, HttpServer or UuidFactory or DeaEnforcingAgent....)
>
> As it is, `QoSCorrection` violates the style guide; please don't do this.
ID in SlaveID is 2 characters long and this can be "legally" put all in caps. ((:
On the other hand, would you prefer QosCorrection than QoSCorrection? In my opinion the latter looks fine (rather everybody knows that *QoS* is the abbreviation), even that we may violate some style guides (e.g Microsoft one). What matters here is what looks better (more understandable) and that depends on someone's opinion..
+1 for consistency with SlaveID, but i agree that kind of violates the common guidelines - maybe we should define it explicitly in http://mesos.apache.org/documentation/latest/mesos-c++-style-guide/ ?
Does it make sense?
- Bartek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review84988
-----------------------------------------------------------
On May 26, 2015, 8:50 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 26, 2015, 8:50 p.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Niklas Nielsen <ni...@qni.dk>.
> On May 22, 2015, 1:50 p.m., Marco Massenzio wrote:
> > include/mesos/slave/oversubscription.proto, line 42
> > <https://reviews.apache.org/r/34581/diff/2/?file=969904#file969904line42>
> >
> > can you define this instead as:
> >
> > ```
> > message ActionInfo {
> > optional ExecutorID executor_id = 1;
> > optional SlaveID slave_id = 2;
> > optional TaskID task_id = 3;
> > }
> > ```
> > or something similar, that makes it more generally applicable?
>
> Bartek Plotka wrote:
> Hey, have you seen the Jie Yu comment in https://reviews.apache.org/r/34571/?
>
> That previous request was as an initial work for this issue - please see. Initially we wanted to do it in more generic way.
> I partly agree with Jie - Offer.Operation is done like that.
>
> Aslo notice that SlaveID is not needed here - the corrections are made in Slave scope.
> Additionaly, we don't want to add unnecessary fields like TaskID for now - if we implement such functionality (killing tasks), then we will add such field
Bartek: +1
Marco: any objections, taken that we will have many actions with different payloads?
> On May 22, 2015, 1:50 p.m., Marco Massenzio wrote:
> > include/mesos/slave/oversubscription.proto, line 34
> > <https://reviews.apache.org/r/34581/diff/2/?file=969904#file969904line34>
> >
> > please consider calling this `QosCorrectiveAction`
> > (we require CamelCase for our types, in any event; so this would have to be `QosCorrection`)
> >
> > I'm also not wild about the `QoS` moniker - I'd like this to be a more generic `CorrectiveAction` message, but happy to go with whatever else others suggest.
>
> Jie Yu wrote:
> I prefer QoSCorrection since QoS is an abbreivation. THis is similar to SlaveID and we don't callid SlaveId :)
+1
> On May 22, 2015, 1:50 p.m., Marco Massenzio wrote:
> > include/mesos/slave/oversubscription.proto, line 49
> > <https://reviews.apache.org/r/34581/diff/2/?file=969904#file969904line49>
> >
> > I have some concerns about this design - given the Note above, this would imply that we would have multiple fields, each with its own message type (eg, `Freeze`, `Resize`, etc. etc.)
> >
> > Can't we just define some sort of base `ActionInfo` type, which may be extensible (maybe, even have an `ExtraInfo`).
> >
> > Been a while since I last played with protobuf at Google, but my concern is the potential growth of fields/types that this approach seem to entail.
>
> Jie Yu wrote:
> This is a union pattern we used consistently in the code base. For example, the new API (include/mesos/scheduler/scheduler.proto), Offer.Operation, etc. I think this is more explicit (thus more readable IMO) comparing to a more general ActionInfo type. What do you think?
Isn't it more confusing to have, for example a resource field in the ActionInfo which only applies to Resize, for example? Being able to have custom fields for different actions was the motivation for this change (we originally proposed to do it in a unified fashion).
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review84988
-----------------------------------------------------------
On May 22, 2015, 2:45 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 22, 2015, 2:45 p.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Bartek Plotka <bw...@gmail.com>.
> On May 22, 2015, 8:50 p.m., Marco Massenzio wrote:
> > include/mesos/slave/oversubscription.proto, line 34
> > <https://reviews.apache.org/r/34581/diff/2/?file=969904#file969904line34>
> >
> > please consider calling this `QosCorrectiveAction`
> > (we require CamelCase for our types, in any event; so this would have to be `QosCorrection`)
> >
> > I'm also not wild about the `QoS` moniker - I'd like this to be a more generic `CorrectiveAction` message, but happy to go with whatever else others suggest.
>
> Jie Yu wrote:
> I prefer QoSCorrection since QoS is an abbreivation. THis is similar to SlaveID and we don't callid SlaveId :)
>
> Niklas Nielsen wrote:
> +1
>
> Marco Massenzio wrote:
> For the record, SlaveID is a violation of the Google Style guide we purportedly follow - it's probably too late to fix now, but we should avoid to perpetuate the mistake.
>
> The rationale of the rule is that it does not leave room for guessing the uppercase/lowercase: it's strictly CamelCase (so, HttpServer or UuidFactory or DeaEnforcingAgent....)
>
> As it is, `QoSCorrection` violates the style guide; please don't do this.
>
> Bartek Plotka wrote:
> ID in SlaveID is 2 characters long and this can be "legally" put all in caps. ((:
>
> On the other hand, would you prefer QosCorrection than QoSCorrection? In my opinion the latter looks fine (rather everybody knows that *QoS* is the abbreviation), even that we may violate some style guides (e.g Microsoft one). What matters here is what looks better (more understandable) and that depends on someone's opinion..
>
> +1 for consistency with SlaveID, but i agree that kind of violates the common guidelines - maybe we should define it explicitly in http://mesos.apache.org/documentation/latest/mesos-c++-style-guide/ ?
>
> Does it make sense?
>
> Marco Massenzio wrote:
> Where did you derive the impression that two-letter all-uppercase are fine?
> https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Type_Names
>
> Exactly because "what looks better [...] depends on someone's opinion" we didn't leave room for interpretation: it's CamelCase, simple as that.
> And having a rule that states the equivalent of "Type Names are all CamelCase, unless you don't like it, in which case it's fine to use whichever case looks better" would be a bit of a challenge, don't you think? :)
>
> Again, I'm not one of the committers, so if they are happy with this, I'll go along with it.
>
> BTW - just FYI: ID is not the abbreviation for I.D. (Identification Document), it's the abbreviation for `Identifier` so the two-letter capital case is just plain wrong (but very widely used). Funnily enough, [UUID](http://en.wikipedia.org/wiki/Universally_unique_identifier) should have actually been UUId, but, hey, whatever :)
https://msdn.microsoft.com/en-us/library/141e06ef(v=vs.71).aspx. I know it's MS but google style guide has no rule for 2 chars long acronyms/abbreviations and it's very common.
"would be a bit of a challenge, don't you think? :)" - totally agree (:
I.D... Wow i wasn't aware of that! =D So do you agree that ID is an *acronym* not *abbreviations*? If that's true then this also a valid case. (:
- Bartek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review84988
-----------------------------------------------------------
On May 26, 2015, 10:24 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 26, 2015, 10:24 p.m.)
>
>
> Review request for mesos, Jie Yu, Joris Van Remoortere, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Bartek Plotka <bw...@gmail.com>.
> On May 22, 2015, 8:50 p.m., Marco Massenzio wrote:
> > include/mesos/slave/oversubscription.proto, line 30
> > <https://reviews.apache.org/r/34581/diff/2/?file=969904#file969904line30>
> >
> > nit: s/correction/corrective action
> >
> > also, prefer "needs to be taken"
Agree.
Also there is gramma mistake in my comment -> s/corrections/correction
> On May 22, 2015, 8:50 p.m., Marco Massenzio wrote:
> > include/mesos/slave/oversubscription.proto, line 42
> > <https://reviews.apache.org/r/34581/diff/2/?file=969904#file969904line42>
> >
> > can you define this instead as:
> >
> > ```
> > message ActionInfo {
> > optional ExecutorID executor_id = 1;
> > optional SlaveID slave_id = 2;
> > optional TaskID task_id = 3;
> > }
> > ```
> > or something similar, that makes it more generally applicable?
Hey, have you seen the Jie Yu comment in https://reviews.apache.org/r/34571/?
That previous request was as an initial work for this issue - please see. Initially we wanted to do it in more generic way.
I partly agree with Jie - Offer.Operation is done like that.
Aslo notice that SlaveID is not needed here - the corrections are made in Slave scope.
Additionaly, we don't want to add unnecessary fields like TaskID for now - if we implement such functionality (killing tasks), then we will add such field
- Bartek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review84988
-----------------------------------------------------------
On May 22, 2015, 7:46 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 22, 2015, 7:46 p.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Jie Yu <yu...@gmail.com>.
> On May 22, 2015, 8:50 p.m., Marco Massenzio wrote:
> > include/mesos/slave/oversubscription.proto, line 49
> > <https://reviews.apache.org/r/34581/diff/2/?file=969904#file969904line49>
> >
> > I have some concerns about this design - given the Note above, this would imply that we would have multiple fields, each with its own message type (eg, `Freeze`, `Resize`, etc. etc.)
> >
> > Can't we just define some sort of base `ActionInfo` type, which may be extensible (maybe, even have an `ExtraInfo`).
> >
> > Been a while since I last played with protobuf at Google, but my concern is the potential growth of fields/types that this approach seem to entail.
This is a union pattern we used consistently in the code base. For example, the new API (include/mesos/scheduler/scheduler.proto), Offer.Operation, etc. I think this is more explicit (thus more readable IMO) comparing to a more general ActionInfo type. What do you think?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review84988
-----------------------------------------------------------
On May 22, 2015, 7:46 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 22, 2015, 7:46 p.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On May 22, 2015, 8:50 p.m., Marco Massenzio wrote:
> > include/mesos/slave/oversubscription.proto, line 34
> > <https://reviews.apache.org/r/34581/diff/2/?file=969904#file969904line34>
> >
> > please consider calling this `QosCorrectiveAction`
> > (we require CamelCase for our types, in any event; so this would have to be `QosCorrection`)
> >
> > I'm also not wild about the `QoS` moniker - I'd like this to be a more generic `CorrectiveAction` message, but happy to go with whatever else others suggest.
>
> Jie Yu wrote:
> I prefer QoSCorrection since QoS is an abbreivation. THis is similar to SlaveID and we don't callid SlaveId :)
>
> Niklas Nielsen wrote:
> +1
>
> Marco Massenzio wrote:
> For the record, SlaveID is a violation of the Google Style guide we purportedly follow - it's probably too late to fix now, but we should avoid to perpetuate the mistake.
>
> The rationale of the rule is that it does not leave room for guessing the uppercase/lowercase: it's strictly CamelCase (so, HttpServer or UuidFactory or DeaEnforcingAgent....)
>
> As it is, `QoSCorrection` violates the style guide; please don't do this.
>
> Bartek Plotka wrote:
> ID in SlaveID is 2 characters long and this can be "legally" put all in caps. ((:
>
> On the other hand, would you prefer QosCorrection than QoSCorrection? In my opinion the latter looks fine (rather everybody knows that *QoS* is the abbreviation), even that we may violate some style guides (e.g Microsoft one). What matters here is what looks better (more understandable) and that depends on someone's opinion..
>
> +1 for consistency with SlaveID, but i agree that kind of violates the common guidelines - maybe we should define it explicitly in http://mesos.apache.org/documentation/latest/mesos-c++-style-guide/ ?
>
> Does it make sense?
Where did you derive the impression that two-letter all-uppercase are fine?
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Type_Names
Exactly because "what looks better [...] depends on someone's opinion" we didn't leave room for interpretation: it's CamelCase, simple as that.
And having a rule that states the equivalent of "Type Names are all CamelCase, unless you don't like it, in which case it's fine to use whichever case looks better" would be a bit of a challenge, don't you think? :)
Again, I'm not one of the committers, so if they are happy with this, I'll go along with it.
BTW - just FYI: ID is not the abbreviation for I.D. (Identification Document), it's the abbreviation for `Identifier` so the two-letter capital case is just plain wrong (but very widely used). Funnily enough, [UUID](http://en.wikipedia.org/wiki/Universally_unique_identifier) should have actually been UUId, but, hey, whatever :)
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review84988
-----------------------------------------------------------
On May 26, 2015, 10:24 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 26, 2015, 10:24 p.m.)
>
>
> Review request for mesos, Jie Yu, Joris Van Remoortere, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Jie Yu <yu...@gmail.com>.
> On May 22, 2015, 8:50 p.m., Marco Massenzio wrote:
> > include/mesos/slave/oversubscription.proto, line 34
> > <https://reviews.apache.org/r/34581/diff/2/?file=969904#file969904line34>
> >
> > please consider calling this `QosCorrectiveAction`
> > (we require CamelCase for our types, in any event; so this would have to be `QosCorrection`)
> >
> > I'm also not wild about the `QoS` moniker - I'd like this to be a more generic `CorrectiveAction` message, but happy to go with whatever else others suggest.
I prefer QoSCorrection since QoS is an abbreivation. THis is similar to SlaveID and we don't callid SlaveId :)
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review84988
-----------------------------------------------------------
On May 22, 2015, 9:45 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 22, 2015, 9:45 p.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On May 22, 2015, 8:50 p.m., Marco Massenzio wrote:
> > include/mesos/slave/oversubscription.proto, line 34
> > <https://reviews.apache.org/r/34581/diff/2/?file=969904#file969904line34>
> >
> > please consider calling this `QosCorrectiveAction`
> > (we require CamelCase for our types, in any event; so this would have to be `QosCorrection`)
> >
> > I'm also not wild about the `QoS` moniker - I'd like this to be a more generic `CorrectiveAction` message, but happy to go with whatever else others suggest.
>
> Jie Yu wrote:
> I prefer QoSCorrection since QoS is an abbreivation. THis is similar to SlaveID and we don't callid SlaveId :)
>
> Niklas Nielsen wrote:
> +1
For the record, SlaveID is a violation of the Google Style guide we purportedly follow - it's probably too late to fix now, but we should avoid to perpetuate the mistake.
The rationale of the rule is that it does not leave room for guessing the uppercase/lowercase: it's strictly CamelCase (so, HttpServer or UuidFactory or DeaEnforcingAgent....)
As it is, `QoSCorrection` violates the style guide; please don't do this.
> On May 22, 2015, 8:50 p.m., Marco Massenzio wrote:
> > include/mesos/slave/oversubscription.proto, line 42
> > <https://reviews.apache.org/r/34581/diff/2/?file=969904#file969904line42>
> >
> > can you define this instead as:
> >
> > ```
> > message ActionInfo {
> > optional ExecutorID executor_id = 1;
> > optional SlaveID slave_id = 2;
> > optional TaskID task_id = 3;
> > }
> > ```
> > or something similar, that makes it more generally applicable?
>
> Bartek Plotka wrote:
> Hey, have you seen the Jie Yu comment in https://reviews.apache.org/r/34571/?
>
> That previous request was as an initial work for this issue - please see. Initially we wanted to do it in more generic way.
> I partly agree with Jie - Offer.Operation is done like that.
>
> Aslo notice that SlaveID is not needed here - the corrections are made in Slave scope.
> Additionaly, we don't want to add unnecessary fields like TaskID for now - if we implement such functionality (killing tasks), then we will add such field
>
> Niklas Nielsen wrote:
> Bartek: +1
>
> Marco: any objections, taken that we will have many actions with different payloads?
Protocol buffer design is a delicate task - it essentially mortgages your future :)
Once a decision is made, you gotta live with it (see, for reference, `MasterInfo`, where the `ip` field was chosen to be `required` and an `int32` field: now, we have to keep it and maintain it - even though it's clearly limiting and there's no way we can support IPv6 with that.
In that respect, I'm cautious when I see statements such as "we'll add this" and "we'll change that" - as that may be an impervious path.
I don't know (or understand) deeply enough about the matter here, to be able to really say one way or another: I'm just cautioning you about the limited room for manouver that PBs give you regarding refactorings and future change.
I have, indeed, seen the comment in the other review (admittedly, after I originally commented on this one) - I can't really say it entirely cleared my doubts.
If everyone agrees that this is a good design, I'm happy to go along (I don't feel strongly enough to block progress) but I want at least people to be aware of potential issues down the road.
> On May 22, 2015, 8:50 p.m., Marco Massenzio wrote:
> > include/mesos/slave/oversubscription.proto, line 49
> > <https://reviews.apache.org/r/34581/diff/2/?file=969904#file969904line49>
> >
> > I have some concerns about this design - given the Note above, this would imply that we would have multiple fields, each with its own message type (eg, `Freeze`, `Resize`, etc. etc.)
> >
> > Can't we just define some sort of base `ActionInfo` type, which may be extensible (maybe, even have an `ExtraInfo`).
> >
> > Been a while since I last played with protobuf at Google, but my concern is the potential growth of fields/types that this approach seem to entail.
>
> Jie Yu wrote:
> This is a union pattern we used consistently in the code base. For example, the new API (include/mesos/scheduler/scheduler.proto), Offer.Operation, etc. I think this is more explicit (thus more readable IMO) comparing to a more general ActionInfo type. What do you think?
>
> Niklas Nielsen wrote:
> Isn't it more confusing to have, for example a resource field in the ActionInfo which only applies to Resize, for example? Being able to have custom fields for different actions was the motivation for this change (we originally proposed to do it in a unified fashion).
I think my confusion comes from my not being familiar with the "union pattern" Jie mentions, sorry.
My current understanding is that, in code, the current design would translate into an if-castle (or giant switch) like this?
```
if type == Kill:
do kill executor_id
else if type == Resize:
do resize executor_id, bytes, ...
else if type == Foo:
do foo a, b, c
...
```
I'm probably completely wrong here, though!
The design I'm advocating would translate into designing a generic interface and then instantiating an appropriate class depending on the action type.
Using variadic templates and lambdas (ask MPark - he's the expert :) this would code rather nicely in C++ (and even Java, using DI, for that matter).
Also, if we were to actually use injection, we could have "custom actions" added that would not require us to modify (and recompile) the PB (and ALL the dependent classes) by just implementing the interface I mention above.
Again, I don't want to block progress, so I'm happy to go with the majority.
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review84988
-----------------------------------------------------------
On May 23, 2015, 3:30 a.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 23, 2015, 3:30 a.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review84988
-----------------------------------------------------------
Thanks for doing this!
Minor nits (and a design concern) but otherwise looks good.
include/mesos/slave/oversubscription.proto
<https://reviews.apache.org/r/34581/#comment136449>
nit: s/correction/corrective action
also, prefer "needs to be taken"
include/mesos/slave/oversubscription.proto
<https://reviews.apache.org/r/34581/#comment136451>
please consider calling this `QosCorrectiveAction`
(we require CamelCase for our types, in any event; so this would have to be `QosCorrection`)
I'm also not wild about the `QoS` moniker - I'd like this to be a more generic `CorrectiveAction` message, but happy to go with whatever else others suggest.
include/mesos/slave/oversubscription.proto
<https://reviews.apache.org/r/34581/#comment136450>
can you define this instead as:
```
message ActionInfo {
optional ExecutorID executor_id = 1;
optional SlaveID slave_id = 2;
optional TaskID task_id = 3;
}
```
or something similar, that makes it more generally applicable?
include/mesos/slave/oversubscription.proto
<https://reviews.apache.org/r/34581/#comment136447>
I have some concerns about this design - given the Note above, this would imply that we would have multiple fields, each with its own message type (eg, `Freeze`, `Resize`, etc. etc.)
Can't we just define some sort of base `ActionInfo` type, which may be extensible (maybe, even have an `ExtraInfo`).
Been a while since I last played with protobuf at Google, but my concern is the potential growth of fields/types that this approach seem to entail.
src/Makefile.am
<https://reviews.apache.org/r/34581/#comment136452>
not sure whether this is an artifact of RB, but your tabs seem to be out of line?
- Marco Massenzio
On May 22, 2015, 7:46 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 22, 2015, 7:46 p.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review84984
-----------------------------------------------------------
Patch looks great!
Reviews applied: [34581]
All tests passed.
- Mesos ReviewBot
On May 22, 2015, 7:46 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 22, 2015, 7:46 p.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review85016
-----------------------------------------------------------
Patch looks great!
Reviews applied: [34581]
All tests passed.
- Mesos ReviewBot
On May 22, 2015, 9:45 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 22, 2015, 9:45 p.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review85045
-----------------------------------------------------------
Patch looks great!
Reviews applied: [34581]
All tests passed.
- Mesos ReviewBot
On May 23, 2015, 12:40 a.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 23, 2015, 12:40 a.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On May 23, 2015, 3:35 p.m., Niklas Nielsen wrote:
> > LGTM!
> >
> > Marco - mind taking a look at the comments and see if there are more things we need to discuss?
done, thanks
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review85067
-----------------------------------------------------------
On May 23, 2015, 3:30 a.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 23, 2015, 3:30 a.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review85067
-----------------------------------------------------------
Ship it!
LGTM!
Marco - mind taking a look at the comments and see if there are more things we need to discuss?
- Niklas Nielsen
On May 22, 2015, 8:30 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 22, 2015, 8:30 p.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review85055
-----------------------------------------------------------
Patch looks great!
Reviews applied: [34581]
All tests passed.
- Mesos ReviewBot
On May 23, 2015, 3:30 a.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 23, 2015, 3:30 a.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Bartek Plotka <bw...@gmail.com>.
> On May 26, 2015, 7:26 p.m., Vinod Kone wrote:
> > include/mesos/slave/oversubscription.proto, line 47
> > <https://reviews.apache.org/r/34581/diff/6/?file=970357#file970357line47>
> >
> > why optional?
>
> Bartek Plotka wrote:
> IMO, because we can (only optionally) specify the descriptive reason of some action.
>
> Secondly, it's not good idea to limit ourselves with too many required fields - what if we will decide in the future that some actions will have some special type of reasons? (:
>
> Niklas Nielsen wrote:
> I don't have a strong opinion on this, other than what Bartek said. Required is forever. Vinod, still want it to be required?
>
> Vinod Kone wrote:
> optional is fine. can it be an enum though? if yes, that would make it easier expose stats on it.
>
> Niklas Nielsen wrote:
> Don't think we can (yet) - we need more experience with the controller before generalizing the reason. Should we call it 'message' instead and plan for a 'reason' enum?
What about dropping that field, since we don't know the particular definition of that reason? It would be difficult to built the enum *reason* for now.. The *reason* will be very useful for some advanced alghoritms like smart corrective units or machine learning, so let's add it when we need such special msg?
- Bartek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review85227
-----------------------------------------------------------
On May 26, 2015, 10:24 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 26, 2015, 10:24 p.m.)
>
>
> Review request for mesos, Jie Yu, Joris Van Remoortere, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Bartek Plotka <bw...@gmail.com>.
> On May 26, 2015, 7:26 p.m., Vinod Kone wrote:
> > include/mesos/slave/oversubscription.proto, line 35
> > <https://reviews.apache.org/r/34581/diff/6/?file=970357#file970357line35>
> >
> > s/future/the future/
Agree.
> On May 26, 2015, 7:26 p.m., Vinod Kone wrote:
> > include/mesos/slave/oversubscription.proto, line 47
> > <https://reviews.apache.org/r/34581/diff/6/?file=970357#file970357line47>
> >
> > why optional?
IMO, because we can (only optionally) specify the descriptive reason of some action.
Secondly, it's not good idea to limit ourselves with too many required fields - what if we will decide in the future that some actions will have some special type of reasons? (:
> On May 26, 2015, 7:26 p.m., Vinod Kone wrote:
> > include/mesos/slave/oversubscription.proto, line 48
> > <https://reviews.apache.org/r/34581/diff/6/?file=970357#file970357line48>
> >
> > why a timestamp?
It could be necessary for some advanced long term corrective actions (e.g "migrate" or "resize"), when you will have to decide if particular correction is up-to-date.
- Bartek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review85227
-----------------------------------------------------------
On May 23, 2015, 3:30 a.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 23, 2015, 3:30 a.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Niklas Nielsen <ni...@qni.dk>.
> On May 26, 2015, 12:26 p.m., Vinod Kone wrote:
> > include/mesos/slave/oversubscription.proto, line 48
> > <https://reviews.apache.org/r/34581/diff/6/?file=970357#file970357line48>
> >
> > why a timestamp?
>
> Bartek Plotka wrote:
> It could be necessary for some advanced long term corrective actions (e.g "migrate" or "resize"), when you will have to decide if particular correction is up-to-date.
>
> Vinod Kone wrote:
> I would suggest to add fields as and when we have concrete requirements.
Alright - sg
> On May 26, 2015, 12:26 p.m., Vinod Kone wrote:
> > include/mesos/slave/oversubscription.proto, line 47
> > <https://reviews.apache.org/r/34581/diff/6/?file=970357#file970357line47>
> >
> > why optional?
>
> Bartek Plotka wrote:
> IMO, because we can (only optionally) specify the descriptive reason of some action.
>
> Secondly, it's not good idea to limit ourselves with too many required fields - what if we will decide in the future that some actions will have some special type of reasons? (:
I don't have a strong opinion on this, other than what Bartek said. Required is forever. Vinod, still want it to be required?
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review85227
-----------------------------------------------------------
On May 26, 2015, 2:09 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 26, 2015, 2:09 p.m.)
>
>
> Review request for mesos, Jie Yu, Joris Van Remoortere, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Vinod Kone <vi...@gmail.com>.
> On May 26, 2015, 7:26 p.m., Vinod Kone wrote:
> > include/mesos/slave/oversubscription.proto, line 47
> > <https://reviews.apache.org/r/34581/diff/6/?file=970357#file970357line47>
> >
> > why optional?
>
> Bartek Plotka wrote:
> IMO, because we can (only optionally) specify the descriptive reason of some action.
>
> Secondly, it's not good idea to limit ourselves with too many required fields - what if we will decide in the future that some actions will have some special type of reasons? (:
>
> Niklas Nielsen wrote:
> I don't have a strong opinion on this, other than what Bartek said. Required is forever. Vinod, still want it to be required?
>
> Vinod Kone wrote:
> optional is fine. can it be an enum though? if yes, that would make it easier expose stats on it.
>
> Niklas Nielsen wrote:
> Don't think we can (yet) - we need more experience with the controller before generalizing the reason. Should we call it 'message' instead and plan for a 'reason' enum?
>
> Bartek Plotka wrote:
> What about dropping that field, since we don't know the particular definition of that reason? It would be difficult to built the enum *reason* for now.. The *reason* will be very useful for some advanced alghoritms like smart corrective units or machine learning, so let's add it when we need such special msg?
+1 to drop.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review85227
-----------------------------------------------------------
On May 26, 2015, 10:24 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 26, 2015, 10:24 p.m.)
>
>
> Review request for mesos, Jie Yu, Joris Van Remoortere, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Niklas Nielsen <ni...@qni.dk>.
> On May 26, 2015, 12:26 p.m., Vinod Kone wrote:
> > include/mesos/slave/oversubscription.proto, line 47
> > <https://reviews.apache.org/r/34581/diff/6/?file=970357#file970357line47>
> >
> > why optional?
>
> Bartek Plotka wrote:
> IMO, because we can (only optionally) specify the descriptive reason of some action.
>
> Secondly, it's not good idea to limit ourselves with too many required fields - what if we will decide in the future that some actions will have some special type of reasons? (:
>
> Niklas Nielsen wrote:
> I don't have a strong opinion on this, other than what Bartek said. Required is forever. Vinod, still want it to be required?
>
> Vinod Kone wrote:
> optional is fine. can it be an enum though? if yes, that would make it easier expose stats on it.
Don't think we can (yet) - we need more experience with the controller before generalizing the reason. Should we call it 'message' instead and plan for a 'reason' enum?
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review85227
-----------------------------------------------------------
On May 26, 2015, 3:24 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 26, 2015, 3:24 p.m.)
>
>
> Review request for mesos, Jie Yu, Joris Van Remoortere, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Vinod Kone <vi...@gmail.com>.
> On May 26, 2015, 7:26 p.m., Vinod Kone wrote:
> > include/mesos/slave/oversubscription.proto, line 48
> > <https://reviews.apache.org/r/34581/diff/6/?file=970357#file970357line48>
> >
> > why a timestamp?
>
> Bartek Plotka wrote:
> It could be necessary for some advanced long term corrective actions (e.g "migrate" or "resize"), when you will have to decide if particular correction is up-to-date.
I would suggest to add fields as and when we have concrete requirements.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review85227
-----------------------------------------------------------
On May 23, 2015, 3:30 a.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 23, 2015, 3:30 a.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Vinod Kone <vi...@gmail.com>.
> On May 26, 2015, 7:26 p.m., Vinod Kone wrote:
> > include/mesos/slave/oversubscription.proto, line 47
> > <https://reviews.apache.org/r/34581/diff/6/?file=970357#file970357line47>
> >
> > why optional?
>
> Bartek Plotka wrote:
> IMO, because we can (only optionally) specify the descriptive reason of some action.
>
> Secondly, it's not good idea to limit ourselves with too many required fields - what if we will decide in the future that some actions will have some special type of reasons? (:
>
> Niklas Nielsen wrote:
> I don't have a strong opinion on this, other than what Bartek said. Required is forever. Vinod, still want it to be required?
optional is fine. can it be an enum though? if yes, that would make it easier expose stats on it.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review85227
-----------------------------------------------------------
On May 26, 2015, 9:09 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 26, 2015, 9:09 p.m.)
>
>
> Review request for mesos, Jie Yu, Joris Van Remoortere, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review85227
-----------------------------------------------------------
include/mesos/slave/oversubscription.proto
<https://reviews.apache.org/r/34581/#comment136744>
s/future/the future/
include/mesos/slave/oversubscription.proto
<https://reviews.apache.org/r/34581/#comment136745>
s/future/the future/
include/mesos/slave/oversubscription.proto
<https://reviews.apache.org/r/34581/#comment136746>
why optional?
include/mesos/slave/oversubscription.proto
<https://reviews.apache.org/r/34581/#comment136743>
why a timestamp?
- Vinod Kone
On May 23, 2015, 3:30 a.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 23, 2015, 3:30 a.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Bartek Plotka <bw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/
-----------------------------------------------------------
(Updated May 26, 2015, 9:07 p.m.)
Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
Changes
-------
Added framework ID for Kill action.
Bugs: MESOS-2760
https://issues.apache.org/jira/browse/MESOS-2760
Repository: mesos
Description
-------
This proto describes a QoS correction message for particular executor or task.
It is a generic message between QoS Controller and slave.
Additionaly, updated Makefile to include this proto during compilation.
This request updates the https://reviews.apache.org/r/34571/
Diffs (updated)
-----
include/mesos/slave/oversubscription.proto PRE-CREATION
src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19
Diff: https://reviews.apache.org/r/34581/diff/
Testing
-------
* make check
* run mesos:
1) build (make)
2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
Thanks,
Bartek Plotka
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review85252
-----------------------------------------------------------
include/mesos/slave/oversubscription.proto
<https://reviews.apache.org/r/34581/#comment136781>
We need the framework id too, as executor id's are only unique per-framework
- Niklas Nielsen
On May 26, 2015, 1:50 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 26, 2015, 1:50 p.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Bartek Plotka <bw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/
-----------------------------------------------------------
(Updated May 26, 2015, 8:50 p.m.)
Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
Changes
-------
s/future/the future/
Bugs: MESOS-2760
https://issues.apache.org/jira/browse/MESOS-2760
Repository: mesos
Description
-------
This proto describes a QoS correction message for particular executor or task.
It is a generic message between QoS Controller and slave.
Additionaly, updated Makefile to include this proto during compilation.
This request updates the https://reviews.apache.org/r/34571/
Diffs (updated)
-----
include/mesos/slave/oversubscription.proto PRE-CREATION
src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19
Diff: https://reviews.apache.org/r/34581/diff/
Testing
-------
* make check
* run mesos:
1) build (make)
2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
Thanks,
Bartek Plotka
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Bartek Plotka <bw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/
-----------------------------------------------------------
(Updated May 23, 2015, 3:30 a.m.)
Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
Changes
-------
Sorry for previous patchset - my branch was not up-to-date.
Bugs: MESOS-2760
https://issues.apache.org/jira/browse/MESOS-2760
Repository: mesos
Description
-------
This proto describes a QoS correction message for particular executor or task.
It is a generic message between QoS Controller and slave.
Additionaly, updated Makefile to include this proto during compilation.
This request updates the https://reviews.apache.org/r/34571/
Diffs (updated)
-----
include/mesos/slave/oversubscription.proto PRE-CREATION
src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
Diff: https://reviews.apache.org/r/34581/diff/
Testing
-------
* make check
* run mesos:
1) build (make)
2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
Thanks,
Bartek Plotka
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Bartek Plotka <bw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/
-----------------------------------------------------------
(Updated May 23, 2015, 3:27 a.m.)
Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
Changes
-------
s/package mesos.internal/package mesos.slave/
Bugs: MESOS-2760
https://issues.apache.org/jira/browse/MESOS-2760
Repository: mesos
Description
-------
This proto describes a QoS correction message for particular executor or task.
It is a generic message between QoS Controller and slave.
Additionaly, updated Makefile to include this proto during compilation.
This request updates the https://reviews.apache.org/r/34571/
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp 215007b7ba8c4093ce95b79a07fd84445048b58a
3rdparty/libprocess/3rdparty/stout/include/stout/hashset.hpp 75ed9db54dc9ab502e978f06c55a621cacb56b91
3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp a538fb1a343aab039aecabe508b7747e683fd46e
3rdparty/libprocess/3rdparty/stout/include/stout/os/stat.hpp c8203d6d1ec41c1c27d139b469a21453183c4903
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 12b2e1b67df30ba4bfdbe12289ee42db8d381954
3rdparty/libprocess/include/process/process.hpp 79d1719932a3fdc90b6247d3a77adee123e72435
include/mesos/master/allocator.hpp 8347cfaf6abce155a777484970e595e6b141bb87
include/mesos/slave/oversubscription.proto PRE-CREATION
src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
src/linux/ns.hpp 6876967f4182e0cd0bc11fe124382629107eebf7
src/master/allocator/mesos/allocator.hpp b57b03daf992082ec7c731666699ab2574bf7993
src/master/allocator/mesos/hierarchical.hpp 44fbccaf72b65251095f69b68627d0efa58707d4
src/master/allocator/sorter/drf/sorter.hpp 35dc1a4d0b5e61b26a07c2c53583d75896aff27c
src/master/allocator/sorter/drf/sorter.cpp ac05afdc7d408735dd796faa68c943e75540aaa7
src/master/allocator/sorter/sorter.hpp 9f7d3ccfd0994be8d562fd5efaeeb9403cf9ce94
src/slave/containerizer/linux_launcher.cpp 8eae258d81229e19f8c587f5e023b1df7deed025
src/tests/hierarchical_allocator_tests.cpp 85bb29e7cfc00579ff8f6d62d6c75e1b99ffcdba
src/tests/mesos.hpp b8f7a2f9236166e42421d926718af8d45e857eba
src/tests/sorter_tests.cpp 435e0bfeb28c1a9ea124312a8b97a869945ac87f
Diff: https://reviews.apache.org/r/34581/diff/
Testing
-------
* make check
* run mesos:
1) build (make)
2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
Thanks,
Bartek Plotka
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review85053
-----------------------------------------------------------
include/mesos/slave/oversubscription.proto
<https://reviews.apache.org/r/34581/#comment136524>
Let's make this 'package mesos.slave' instead. See allocator.proto
- Niklas Nielsen
On May 22, 2015, 5:40 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
>
> (Updated May 22, 2015, 5:40 p.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Bugs: MESOS-2760
> https://issues.apache.org/jira/browse/MESOS-2760
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This proto describes a QoS correction message for particular executor or task.
> It is a generic message between QoS Controller and slave.
>
> Additionaly, updated Makefile to include this proto during compilation.
>
> This request updates the https://reviews.apache.org/r/34571/
>
>
> Diffs
> -----
>
> include/mesos/slave/oversubscription.proto PRE-CREATION
> src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
>
> Diff: https://reviews.apache.org/r/34581/diff/
>
>
> Testing
> -------
>
> * make check
> * run mesos:
> 1) build (make)
> 2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
> 3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Bartek Plotka <bw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/
-----------------------------------------------------------
(Updated May 23, 2015, 12:40 a.m.)
Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
Changes
-------
QosCorrection -> QoSCorrection
Bugs: MESOS-2760
https://issues.apache.org/jira/browse/MESOS-2760
Repository: mesos
Description
-------
This proto describes a QoS correction message for particular executor or task.
It is a generic message between QoS Controller and slave.
Additionaly, updated Makefile to include this proto during compilation.
This request updates the https://reviews.apache.org/r/34571/
Diffs (updated)
-----
include/mesos/slave/oversubscription.proto PRE-CREATION
src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
Diff: https://reviews.apache.org/r/34581/diff/
Testing
-------
* make check
* run mesos:
1) build (make)
2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
Thanks,
Bartek Plotka
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Bartek Plotka <bw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/
-----------------------------------------------------------
(Updated May 22, 2015, 9:45 p.m.)
Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
Changes
-------
1) FIXED > Marco Massenzio: nit: s/correction/corrective action
also, prefer "needs to be taken"
2) FIXED > Marco Massenzio: not sure whether this is an artifact of RB, but your tabs seem to be out of line?
Bugs: MESOS-2760
https://issues.apache.org/jira/browse/MESOS-2760
Repository: mesos
Description
-------
This proto describes a QoS correction message for particular executor or task.
It is a generic message between QoS Controller and slave.
Additionaly, updated Makefile to include this proto during compilation.
This request updates the https://reviews.apache.org/r/34571/
Diffs (updated)
-----
include/mesos/slave/oversubscription.proto PRE-CREATION
src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
Diff: https://reviews.apache.org/r/34581/diff/
Testing
-------
* make check
* run mesos:
1) build (make)
2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
Thanks,
Bartek Plotka
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Bartek Plotka <bw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/
-----------------------------------------------------------
(Updated May 22, 2015, 7:46 p.m.)
Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
Changes
-------
Addressed all comments
Bugs: MESOS-2760
https://issues.apache.org/jira/browse/MESOS-2760
Repository: mesos
Description
-------
This proto describes a QoS correction message for particular executor or task.
It is a generic message between QoS Controller and slave.
Additionaly, updated Makefile to include this proto during compilation.
This request updates the https://reviews.apache.org/r/34571/
Diffs (updated)
-----
include/mesos/slave/oversubscription.proto PRE-CREATION
src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
Diff: https://reviews.apache.org/r/34581/diff/
Testing
-------
* make check
* run mesos:
1) build (make)
2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
Thanks,
Bartek Plotka
Re: Review Request 34581: Added oversubscription.proto for QoS
Controller and slave communication
Posted by Bartek Plotka <bw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/
-----------------------------------------------------------
(Updated May 22, 2015, 2:31 a.m.)
Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
Bugs: MESOS-2760
https://issues.apache.org/jira/browse/MESOS-2760
Repository: mesos
Description
-------
This proto describes a QoS correction message for particular executor or task.
It is a generic message between QoS Controller and slave.
Additionaly, updated Makefile to include this proto during compilation.
This request updates the https://reviews.apache.org/r/34571/
Diffs
-----
include/mesos/slave/oversubscription.proto PRE-CREATION
src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
Diff: https://reviews.apache.org/r/34581/diff/
Testing (updated)
-------
* make check
* run mesos:
1) build (make)
2) checked that *oversubscription.pb.h* and *oversubscription.pb.cc* are in the proper directories
3) run *mesos-slave*, *mesos-master* and checked if their behaviour is proper
Thanks,
Bartek Plotka