> 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.
Added a TODO for validation. > 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 > > Anand Mazumdar wrote: > 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 ? Removed leading "_" > 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 > > Anand Mazumdar wrote: > 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 ? This method was no longer needed in the recent iteration. Dropping the issue. - 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 > >
