> On July 9, 2015, 10:24 p.m., Marco Massenzio wrote: > > src/common/protobuf_utils.hpp, lines 78-79 > > <https://reviews.apache.org/r/36318/diff/2/?file=1003766#file1003766line78> > > > > please use javadoc format > > also unnecessary semicolon > > > > s/a/an > > (eg "an Event")
any particular reason for not documenting this method in accordance w/ style guide (javadoc)? > 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 :) > > Anand Mazumdar wrote: > 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 ? Yes, please - the more documentation we add, the less we avoid repeating the effort (that you must have just done) of reverse-engineering the code and understanding what it does. Like money and beauty, there is no such thing as too much documentation :) > 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). > > Anand Mazumdar wrote: > 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. > > Anand Mazumdar wrote: > Added a TODO for validation. ok - then please 'drop' the issue (or it looks like you haven't addressed yet). > 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 > > Anand Mazumdar wrote: > 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. same as above, then - add a TODO & drop issue > 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 :) > > Anand Mazumdar wrote: > Same as above. same comment > 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 > > Anand Mazumdar wrote: > 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 ? I do disagree with punting the problem to "a later time" Also adding more work on the poor soul who will have to fix the comments (it's hardly a "sweep" - it requires work and understanding the method(s) + reverse engineering them. Let's start making things The Right Way, and then we can expand the goodness (by all means, feel free to fix other methods' comments too - some, all or none). - Marco ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/#review91213 ----------------------------------------------------------- On July 15, 2015, 4:56 a.m., Anand Mazumdar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36318/ > ----------------------------------------------------------- > > (Updated July 15, 2015, 4:56 a.m.) > > > Review request for mesos, Benjamin Hindman, 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,...) > - FrameworkDriver can be of 2 types http, libprocess. An Optional member > variable is used to distinguiush between them. > - 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 9ac81c38efd70f92c64a5865fa79fe516e84dd92 > src/master/http.cpp 23a6d4bd2f60cb4a4ad463aea7cc032941578bdc > src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec > src/master/master.cpp b877676afa6f3833eb7d2fb06beeaa288bd8bd5d > 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 > >
