> 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
> 
>

Reply via email to