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 Fri, 10 Jul 2015 00:23:26 GMT


> On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
> > Code looks good!
> > A few general comments (please address them across the entire review - I stopped
making them in every instance):
> > 
> > - no need for leading underscor in argument lists, the private members now have
a trailing one, so no danger of confusion;
> > - make sure you align the `<<` in streaming LOGs (same as everywhere in Mesos);
> > - please make sure that **every** public method has a sufficient Doxygen javadoc-formatted
documentation;
> > - while we wait for Isabel's http_constants.hpp file to land, please use a "surrogate"
one, which you can then just replace with an `#include` of the real one
> 
> Ben Mahler wrote:
>     I'll shepherd this for Anand, IMO this change needs to go through a higher level
review first, otherwise we're likely to go through a ton of iterations (see part (1) of 'Reviewing'
here):
>     http://mesos.apache.org/documentation/latest/effective-code-reviewing/

Thanks Ben for agreeing to shepherd this. Looking forward to your inputs and discussing the
change in greater detail.


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

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 ?


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

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.


> 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

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


> 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

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 ?


> On July 9, 2015, 10:24 p.m., Marco Massenzio wrote:
> > src/master/http.cpp, lines 1271-1273
> > <https://reviews.apache.org/r/36318/diff/2/?file=1003768#file1003768line1271>
> >
> >     align << (see other code)

My bad, let me fix all of these.


> 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

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 ?


> 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

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.


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

Same as above.


- Anand


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36318/#review91213
-----------------------------------------------------------


On July 9, 2015, 6:49 p.m., Anand Mazumdar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36318/
> -----------------------------------------------------------
> 
> (Updated July 9, 2015, 6:49 p.m.)
> 
> 
> Review request for mesos, 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,...)
> - Framework's can be of 2 types now http, libprocess.
> - Made a adapter class that takes the messages from master, transforms them to events
that the http framework driver can then understand.
> - 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 8a51daa45db312ca4608dda3fd99df2c3f9962f1 
>   src/master/http.cpp 23a6d4bd2f60cb4a4ad463aea7cc032941578bdc 
>   src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec 
>   src/master/master.cpp 35e147574842e3c6ae819541a6c03bf16a375294 
>   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