mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Anand Mazumdar" <mazumdar.an...@gmail.com>
Subject Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
Date Wed, 15 Jul 2015 05:00:24 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message