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 Sat, 09 May 2015 02:27:08 GMT

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

Ship it!


Only minor nits from my side, it otherwise LGTM


include/mesos/executor/executor.hpp
<https://reviews.apache.org/r/33823/#comment133994>

    please avoid SCREAMING COMMENTS :)
    (also, not sure what is the use of that comment? is it a TODO? should we have a #pragma?
what?)



include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment133992>

    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.



include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment133993>

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


- Marco Massenzio


On May 4, 2015, 10:21 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33823/
> -----------------------------------------------------------
> 
> (Updated May 4, 2015, 10:21 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 93c7c8a807a33ab639be6289535bbd32022aa85b 
> 
> 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