mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Niklas Nielsen" <...@qni.dk>
Subject Re: Review Request 34581: Added oversubscription.proto for QoS Controller and slave communication
Date Sat, 23 May 2015 00:31:51 GMT


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


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