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