Re: Review Request 34581: Added oversubscription.proto for QoS Controller and slave communication
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
--- 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
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
--- 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
--- 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
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
--- 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. Changes --- Removed timestamp since, we don't have logic to use that now. 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
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
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
--- 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
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
--- 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
--- 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
--- 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
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
--- 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
--- 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
--- 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
--- 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
--- 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 b57b03daf992082ec7c73199ab2574bf7993 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
--- 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
--- 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
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
