> 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") > > Marco Massenzio wrote: > any particular reason for not documenting this method in accordance w/ > style guide (javadoc)?
Yes, from the style guide : "Doxygen documentation needs only to be applied to source code parts that constitute an interface for which we want to generate Mesos API documentation files. Implementation code that does not participate in this should still be enhanced by source code comments as appropriate, but these comments should not follow the doxygen style." These methods are not related to any public API and hence have non doxygen based comments. I fixed the type you mentioned though. > 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 ? > > Marco Massenzio wrote: > 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 :) Same as above, not an public API method. > 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. > > Marco Massenzio wrote: > ok - then please 'drop' the issue (or it looks like you haven't addressed > yet). Had already added a TODO for validation. > 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. > > Marco Massenzio wrote: > same as above, then - add a TODO & drop issue Had added a TODO for validation. > 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. > > Marco Massenzio wrote: > same comment Added a generic TODO for that this would be handled as part of validation. > 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 ? > > Marco Massenzio wrote: > 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). This is not a public API and hence does not need doxygen based comments. Having the old style comments is enough. - Anand ----------------------------------------------------------- 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 > >
