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 33823: Protocol file with the HTTP API messages between executor and slave
Date Tue, 19 May 2015 20:34:53 GMT


> On May 9, 2015, 2:27 a.m., Marco Massenzio wrote:
> > include/mesos/executor/executor.hpp, line 22
> > <https://reviews.apache.org/r/33823/diff/1/?file=949197#file949197line22>
> >
> >     please avoid SCREAMING COMMENTS :)
> >     (also, not sure what is the use of that comment? is it a TODO? should we have
a #pragma? what?)
> 
> Alexander Rojas wrote:
>     1. It is a warning.
>     2. The purpose of this header is to be a reflection of `include/mesos/scheduler/scheduler.hpp`,
and so they both have the same comments, in fact they look mostly identical which follows
the maxim of being consistent.

Well, two wrongs don't make one right.
Maybe fix both, or at least let's not perpetuate something that maybe was not ideal to start
with.

Please change to something that reads like:

// NOTE: this header only becomes valid after running protoc 
// and generating the equivalent .ph.h files
// See: src/messages/mesos.proto
(or whatever the proto file is)

or something to that effect.
SCREAMING COMMENTS ARE BAD FORM !!!!!! 
:-)

(you could almost hear me screaming, could you? :)))


> On May 9, 2015, 2:27 a.m., Marco Massenzio wrote:
> > include/mesos/executor/executor.proto, line 80
> > <https://reviews.apache.org/r/33823/diff/1/?file=949198#file949198line80>
> >
> >     `message` is becoming a truly overloaded term here: as a Protobuf message, the
Message class, this (string) variable here, the (Message) `message` field below... how about
`errorMessage` instead?
> 
> Alexander Rojas wrote:
>     It is called like that, because it is forwarded from the master and to the master.
While I agree with you in principle, you know we like consistency. Why not discuss it in the
HTTP API meeting?

I don't understand the "consistency" reference.  This is an `error message` and should be
named as such? what is not "consistent" about that?
When we use the same term with (semantically) different meanings, we make our code more difficult
to read and understand - people have to go back and forth - especially when, as in this case,
comments are very few and far between.


- Marco


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


On May 19, 2015, 12:19 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33823/
> -----------------------------------------------------------
> 
> (Updated May 19, 2015, 12:19 p.m.)
> 
> 
> Review request for mesos, Isabel Jimenez, Marco Massenzio, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   include/mesos/executor/executor.hpp PRE-CREATION 
>   include/mesos/executor/executor.proto PRE-CREATION 
>   src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 
> 
> Diff: https://reviews.apache.org/r/33823/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


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