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 Sat, 18 Jul 2015 02:17:45 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")
> 
> Marco Massenzio wrote:
>     any particular reason for not documenting this method in accordance w/ style guide
(javadoc)?

Yes, from the style guide :

"Doxygen documentation needs only to be applied to source code parts that constitute an interface
for which we want to generate Mesos API documentation files. Implementation code that does
not participate in this should still be enhanced by source code comments as appropriate, but
these comments should not follow the doxygen style."

These methods are not related to any public API and hence have non doxygen based comments.
I fixed the type you mentioned though.


> 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 ?
> 
> Marco Massenzio wrote:
>     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 :)

Same as above, not an public API method.


> 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.
> 
> Marco Massenzio wrote:
>     ok - then please 'drop' the issue (or it looks like you haven't addressed yet).

Had already added a TODO for validation.


> 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.
> 
> Marco Massenzio wrote:
>     same as above, then - add a TODO & drop issue

Had added a TODO for validation.


> 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.
> 
> Marco Massenzio wrote:
>     same comment

Added a generic TODO for that this would be handled as part of validation.


> 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 ?
> 
> Marco Massenzio wrote:
>     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).

This is not a public API and hence does not need doxygen based comments. Having the old style
comments is enough.


- 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