> On July 9, 2015, 10:24 p.m., Marco Massenzio wrote: > > 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 > > Ben Mahler wrote: > I'll shepherd this for Anand, IMO this change needs to go through a > higher level review first, otherwise we're likely to go through a ton of > iterations (see part (1) of 'Reviewing' here): > http://mesos.apache.org/documentation/latest/effective-code-reviewing/
Thanks Ben for agreeing to shepherd this. Looking forward to your inputs and discussing the change in greater detail. > On July 9, 2015, 10:24 p.m., Marco Massenzio wrote: > > src/master/http.cpp, line 307 > > <https://reviews.apache.org/r/36318/diff/2/?file=1003768#file1003768line307> > > > > 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 :) There is already a TODO on the CALL_HELP variable for documenting this better. This can easily be pursued as part of that. Do you still want me to pursue this as part of this review ? > On July 9, 2015, 10:24 p.m., Marco Massenzio wrote: > > src/master/http.cpp, lines 311-316 > > <https://reviews.apache.org/r/36318/diff/2/?file=1003768#file1003768line311> > > > > 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 { > > ... > > } > > ``` > > Isabel Jimenez wrote: > Hi @marco I commented on previous review that this valdiations will be > handle in the split of the /call patch. That's why it's missing here. It'll > be added with the rebase. > > Marco Massenzio wrote: > I'm not entirely sure I understand (but I don't really need to): please > then add either a TODO to check the checks (so to speak) or an inline comment > stating the invariant - but, if so, I would like to see a CHECK_SOME() to > make it explicit. > Otherwise, someone else (yourself in 3 months!) may not know/remember > about this and may add redundant checks and/or or remove the others (and > leave the condition unchecked). My bad, I should have added a better TODO instead of the vague one that says "Fix logic when we start supporting application/json". I will add a better TODO. I was under the impression that all this would be taken care as part of the validations patch. > On July 9, 2015, 10:24 p.m., Marco Massenzio wrote: > > src/master/http.cpp, lines 1246-1249 > > <https://reviews.apache.org/r/36318/diff/2/?file=1003768#file1003768line1246> > > > > can you please avoid the leading underscore? they seem largely > > unnecessary now that we have the trailing ones for the private members >From the style guide : - "We prepend constructor and function arguments with a leading underscore to avoid ambiguity and / or shadowing:" - "Prefer trailing underscores for use as member fields (but not required). Some trailing underscores are used to distinguish between similar variables in the same scope (think prime symbols), but this should be avoided as much as possible, including removing existing instances in the code base." Seems like I am missing something here, I don't find any reason to follow one and discard the other i.e. not use both at once. If not, these need to be better explained in the style guide. What do you think ? > On July 9, 2015, 10:24 p.m., Marco Massenzio wrote: > > src/master/http.cpp, line 1261 > > <https://reviews.apache.org/r/36318/diff/2/?file=1003768#file1003768line1261> > > > > no leading underscore > > please add doxy for method The _ was added as it was needed for resolving ambiguity with the already declared event variable used in the function. Also it already has a explanation of what the method does in the .hpp file base-class. What would we gain by adding documentation again here ? > On July 9, 2015, 10:24 p.m., Marco Massenzio wrote: > > src/master/http.cpp, lines 1271-1273 > > <https://reviews.apache.org/r/36318/diff/2/?file=1003768#file1003768line1271> > > > > align << (see other code) My bad, let me fix all of these. > On July 9, 2015, 10:24 p.m., Marco Massenzio wrote: > > src/master/master.hpp, line 353 > > <https://reviews.apache.org/r/36318/diff/2/?file=1003769#file1003769line353> > > > > use javadoc instead I have a general query regarding using javadoc or doxygen styled comments in already existing files that also follow the old style comment pattern. If you see the comments on CR : https://reviews.apache.org/r/36106 (BenM's comments ). I tend to agree with him here, we can do a later sweep of the entire file. What do you think ? > On July 9, 2015, 10:24 p.m., Marco Massenzio wrote: > > src/master/http.cpp, line 317 > > <https://reviews.apache.org/r/36318/diff/2/?file=1003768#file1003768line317> > > > > the error is actually 415 Media Not Supported (I think - please double > > check) > > Isabel Jimenez wrote: > Same here Re-iterating my earlier comments, all of this needs to be handled as part of a separate validation patch, this patch just has the minor objective of getting subcribed events to work and not handle validations. > On July 9, 2015, 10:24 p.m., Marco Massenzio wrote: > > src/master/http.cpp, line 342 > > <https://reviews.apache.org/r/36318/diff/2/?file=1003768#file1003768line342> > > > > 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 :) Same as above. - Anand ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/#review91213 ----------------------------------------------------------- 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 > >
