mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexander Rojas" <alexan...@mesosphere.io>
Subject Re: Review Request 33823: Protocol file with the HTTP API messages between executor and slave
Date Wed, 20 May 2015 06:42:07 GMT


> On May 9, 2015, 4: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.
> 
> Marco Massenzio wrote:
>     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? :)))

I think that is the point, when someone is going to fall off a cliff you don't politely ask
them to be careful.


> On May 9, 2015, 4: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?
> 
> Marco Massenzio wrote:
>     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.

The consistency argument goes as follows: It is already widely use in the rest of the code
so it is better to continue using it like this. In any case, I brought this issue to the team
meeting and I am dropping it as discussed there.


> On May 9, 2015, 4:27 a.m., Marco Massenzio wrote:
> > include/mesos/executor/executor.proto, line 32
> > <https://reviews.apache.org/r/33823/diff/1/?file=949198#file949198line32>
> >
> >     can we add also a link to the HTTP API design doc? this way for anyone looking
at the Javadoc, they can follow the link and make some sense of the generated classes.
> >     
> >     Also, as a general comment, I'd really like all `message`s to have at least
a brief description, so that the Javadoc that gets generated is meaningful to folks writing
clients.
> >     
> >     As a general rule, we should not expect folks who write against an API to have
to read the code (they may not even be able to easily access it) and, in particular those
writing Java are likely to use an IDE that will present the javadoc in a pop-up/tooltip upon
auto-completion.

Added comments.


- Alexander


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


On May 20, 2015, 8:41 a.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33823/
> -----------------------------------------------------------
> 
> (Updated May 20, 2015, 8:41 a.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