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