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