----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/#review91213 -----------------------------------------------------------
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 one src/common/protobuf_utils.hpp (lines 78 - 79) <https://reviews.apache.org/r/36318/#comment144492> please use javadoc format also unnecessary semicolon s/a/an (eg "an Event") src/common/protobuf_utils.cpp (line 198) <https://reviews.apache.org/r/36318/#comment144493> "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) <https://reviews.apache.org/r/36318/#comment144494> while you are at it, do you mind adding javadoc doxy documentation to this method? 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) <https://reviews.apache.org/r/36318/#comment144497> 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 yet"); } else { ... } ``` src/master/http.cpp (line 317) <https://reviews.apache.org/r/36318/#comment144495> the error is actually 415 Media Not Supported (I think - please double check) src/master/http.cpp (line 342) <https://reviews.apache.org/r/36318/#comment144498> 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) <https://reviews.apache.org/r/36318/#comment144500> 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) <https://reviews.apache.org/r/36318/#comment144501> no leading underscore please add doxy for method src/master/http.cpp (lines 1271 - 1273) <https://reviews.apache.org/r/36318/#comment144502> align << (see other code) src/master/http.cpp (lines 1277 - 1278) <https://reviews.apache.org/r/36318/#comment144503> here too src/master/master.hpp (line 353) <https://reviews.apache.org/r/36318/#comment144504> 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 > >
