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 Tue, 26 May 2015 22:42:34 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
> 
> Marco Massenzio wrote:
>     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.
> 
> Bartek Plotka wrote:
>     ID in SlaveID is 2 characters long and this can be "legally" put all in caps. ((:

>     
>     On the other hand, would you prefer QosCorrection than QoSCorrection? In my opinion
the latter looks fine (rather everybody knows that *QoS* is the abbreviation), even that we
may violate some style guides (e.g Microsoft one). What matters here is what looks better
(more understandable) and that depends on someone's opinion.. 
>     
>     +1 for consistency with SlaveID, but i agree that kind of violates the common guidelines
- maybe we should define it explicitly in http://mesos.apache.org/documentation/latest/mesos-c++-style-guide/
? 
>     
>     Does it make sense?

Where did you derive the impression that two-letter all-uppercase are fine?
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Type_Names

Exactly because "what looks better [...] depends on someone's opinion" we didn't leave room
for interpretation: it's CamelCase, simple as that.
And having a rule that states the equivalent of "Type Names are all CamelCase, unless you
don't like it, in which case it's fine to use whichever case looks better" would be a bit
of a challenge, don't you think? :)

Again, I'm not one of the committers, so if they are happy with this, I'll go along with it.

BTW - just FYI: ID is not the abbreviation for I.D. (Identification Document), it's the abbreviation
for `Identifier` so the two-letter capital case is just plain wrong (but very widely used).
 Funnily enough, [UUID](http://en.wikipedia.org/wiki/Universally_unique_identifier) should
have actually been UUId, but, hey, whatever :)


- Marco


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34581/#review84988
-----------------------------------------------------------


On May 26, 2015, 10:24 p.m., Bartek Plotka wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34581/
> -----------------------------------------------------------
> 
> (Updated May 26, 2015, 10:24 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Joris Van Remoortere, 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 814468e3c5c750a6649b5eeb7c7f945f9e025c19 
> 
> 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