mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marco Massenzio" <ma...@mesosphere.io>
Subject Re: Review Request 34581: Added oversubscription.proto for QoS Controller and slave communication
Date Sat, 23 May 2015 21:20:22 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message