This is an automatically generated e-mail. To reply, visit:

Code looks good!
A few general comments (please address them across the entire review - I 
stopped making them in every instance):

- no need for leading underscor in argument lists, the private members now have 
a trailing one, so no danger of confusion;
- make sure you align the `<<` in streaming LOGs (same as everywhere in Mesos);
- please make sure that **every** public method has a sufficient Doxygen 
javadoc-formatted documentation;
- while we wait for Isabel's http_constants.hpp file to land, please use a 
"surrogate" one, which you can then just replace with an `#include` of the real 

src/common/protobuf_utils.hpp (lines 78 - 79)

    please use javadoc format
    also unnecessary semicolon
    (eg "an Event")

src/common/protobuf_utils.cpp (line 198)

    "an event"
    also the indent looks off to me?
    (either four spaces or line up with quotes - but do you really need to 
break the string?)

src/master/http.cpp (line 307)

    while you are at it, do you mind adding javadoc doxy documentation to this 
    what it does, what the @param's are, what does it return; maybe a link to 
the design doc...
    as much as you feel like, really: like money and beauty, there's no too 
much documentation :)

src/master/http.cpp (lines 311 - 316)

    unless you know for a fact that none of this will be `None()` you *must* 
check, or this will crash Mesos: hence
    if (contentType.isNone()) {
      return BadRequest("Content-Type header MUST be specified");
    } else if (contentType.get() == Constants.APPLICATION_JSON) {
      return MediaNotSupported("We do not support JSON request/response content 
    } else {

src/master/http.cpp (line 317)

    the error is actually 415 Media Not Supported (I think - please double 

src/master/http.cpp (line 342)

    we should return a `BadRequest` here, shouldn't we? or use UNREACHABLE() 
(but that would seem too radical to me: one could crash Mesos with a malformed 
request: yay for DOS :)

src/master/http.cpp (lines 1246 - 1249)

    can you please avoid the leading underscore? they seem largely unnecessary 
now that we have the trailing ones for the private members

src/master/http.cpp (line 1261)

    no leading underscore
    please add doxy for method

src/master/http.cpp (lines 1271 - 1273)

    align << (see other code)

src/master/http.cpp (lines 1277 - 1278)

    here too

src/master/master.hpp (line 353)

    use javadoc instead

- Marco Massenzio

On July 9, 2015, 6:49 p.m., Anand Mazumdar wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36318/
> -----------------------------------------------------------
> (Updated July 9, 2015, 6:49 p.m.)
> Review request for mesos, Ben Mahler, Isabel Jimenez, Marco Massenzio, and 
> Vinod Kone.
> Bugs: MESOS-2294
>     https://issues.apache.org/jira/browse/MESOS-2294
> Repository: mesos
> Description
> -------
> This change lays the ground-work for the master's ability to stream events 
> back to the client. This review turned out a bit too large for my own liking 
> but in a nutshell, it just takes a subscribe request and puts a subscribed 
> event back on the stream.
> Explanation of changes:
> - Made a generic FrameworkDriver interface that the master now uses to 
> communicate with the frameworks instead of just invoking 
> send(framework->pid,...)
> - Framework's can be of 2 types now http, libprocess.
> - Made a adapter class that takes the messages from master, transforms them 
> to events that the http framework driver can then understand.
> - This still uses hard-coded http related constants. They can go away when 
> Isabel submits her validation change (36217)
> - This change prefers use of using trailing under-scores as member variables 
> from the style guide.
> Diffs
> -----
>   src/common/protobuf_utils.hpp afe5a85d3f58eaabb16807253c5fcc07cabcf8e8 
>   src/common/protobuf_utils.cpp 8a51daa45db312ca4608dda3fd99df2c3f9962f1 
>   src/master/http.cpp 23a6d4bd2f60cb4a4ad463aea7cc032941578bdc 
>   src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec 
>   src/master/master.cpp 35e147574842e3c6ae819541a6c03bf16a375294 
>   src/tests/http_api_tests.cpp 64bbeb6699171e85a5be293919ad9f32ded0ebac 
> Diff: https://reviews.apache.org/r/36318/diff/
> Testing
> -------
> make check + a simple test for subscribe call received a subscribed event 
> back on the stream.
> Thanks,
> Anand Mazumdar

Reply via email to