mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marco Massenzio" <ma...@mesosphere.io>
Subject Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
Date Sat, 18 Jul 2015 01:24:01 GMT


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

any particular reason for not documenting this method in accordance w/ style guide (javadoc)?


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

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


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

ok - then please 'drop' the issue (or it looks like you haven't addressed yet).


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

same as above, then - add a TODO & drop issue


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

same comment


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

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


- Marco


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