Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
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 :) Anand Mazumdar wrote: Same as above, not an public API method. Like money and beauty, there is no such thing as too much documentation :) We strive to write code that is clear without needing lots of documenation! Please be thoughtful about documentation, documentation is prone to rot and excessive documentation poses a higher maintenance burden. Ultimately, the code is always the source of truth. That being said, I do agree that we should document our libraries and anything that is not intuitive! While I agree with the intent.. I find this kinds of broad generalization counter-productive to what we're trying to accomplish, clearly if we had 10x the amount of documentation as we have code, we will be crushed under the maintenance burden / verbosity, no? - Ben --- 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
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
On July 21, 2015, 12:58 a.m., Ben Mahler wrote: Thanks Anand! Couple of issues, per our chat on IRC: (1) There is no 'pid' for pure HTTP schedulers, so we'll need to ensure that the master / slave can handle not having a framework pid, which is a bit tricky to get right. Note that the slave directly sends messages to frameworks, so we'll need to make sure that it can differentiate between when it needs to forward to the master vs. directly to the framework. There's also a bunch of code in the master that uses '`framework-pid`' that we'll need to go over and update to handle http schedulers. (2) To continue to support the scheduler driver's message passing optimization while still having a driver that speaks HTTP, we'll need to pass it's PID somehow. Vinod and I were thinking of having a special HTTP header to pass it through. I left some other comments, but let's figure out a plan during the http sync tomorrow. Thanks for the review. We can easily deal with both of the issues i.e. the 'pid of http schedulers'/'scheduler driver message passing' independently in a separate change and should not block this review ? This abstraction for FrameworkDriver already takes care of all the occurences of framework-pid and correctly resolves them to writing the contents on the stream for http schedulers. On July 21, 2015, 12:58 a.m., Ben Mahler wrote: src/common/protobuf_utils.hpp, line 83 https://reviews.apache.org/r/36318/diff/3/?file=1012218#file1012218line83 Why define this? Seems that we would want this case to be a compilation error instead of having an Error at runtime. This has to do with how the FrameworkDriver::send is templated. It's a function that handles both events and messages ( to ensure backward compatibility ). The compiler would barf out if I don't have this generic function that handles a generic message. This would anyways go away when we have implemented support for all the message types we support. On July 21, 2015, 12:58 a.m., Ben Mahler wrote: src/common/protobuf_utils.cpp, lines 57-60 https://reviews.apache.org/r/36318/diff/3/?file=1012219#file1012219line57 Mind fixing this in a separate patch since it's independent? We can get it committed quickly that way. Also, seems like we should have had message as an Option, but let's save that for later. This is a chicken and a egg problem. I need to access the delcared functions in the header file and as soon as I include that, I would need to remove these default argument values. The only reason this worked till now was that someone accidently forgot including the header file. :) Do you want me to ignore including the header file for now too like others did ? On July 21, 2015, 12:58 a.m., Ben Mahler wrote: src/common/protobuf_utils.cpp, lines 193-202 https://reviews.apache.org/r/36318/diff/3/?file=1012219#file1012219line193 Why is this a Try? It seems pretty dangerous for message conversion to fail at run time, was there some errors we need to capture, even if we remove the generic 'Message' parsing error here (move it to compile time error). The only reason for having this as a TryEvent was to make the generic messages that have not been implemented throw an error. Eventually when we have implement all the message types, we can change this to just being events. I can add a TODO for this ? On July 21, 2015, 12:58 a.m., Ben Mahler wrote: src/master/http.cpp, lines 314-325 https://reviews.apache.org/r/36318/diff/3/?file=1012220#file1012220line314 Whoops, this will crash if contentType is none. Mind also being explicit about protobuf, rather than assuming that !json implies protobuf? Also, would you mind pulling out this code into a separate patch? It will make it easier to land incrementally :) I had already added a TODO for this // Add validations for Content-Type, Accept headers being present. This is being worked upon by Isabel as part of the validations change. I can add a separate TODO for being more explicit about protobuf ? On July 21, 2015, 12:58 a.m., Ben Mahler wrote: src/master/http.cpp, line 346 https://reviews.apache.org/r/36318/diff/3/?file=1012220#file1012220line346 Not yours, but a 501 seems more appropriate here for now? Sure, I can make the change. On July 21, 2015, 12:58 a.m., Ben Mahler wrote: src/master/master.cpp, line 1747 https://reviews.apache.org/r/36318/diff/3/?file=101#file101line1747 frameworkInfo.name() is not equivalent to the framework's pid. Per our chat on IRC, we won't have the PID for pure HTTP frameworks, which means that the slave needs to send messages through the master, and we'll probably need to re-work some more of the master code as well. We can easily carry out the refactoring to not need framework pid as part of another change. This change allows us to get the basic set up
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/#review92329 --- Thanks Anand! Couple of issues, per our chat on IRC: (1) There is no 'pid' for pure HTTP schedulers, so we'll need to ensure that the master / slave can handle not having a framework pid, which is a bit tricky to get right. Note that the slave directly sends messages to frameworks, so we'll need to make sure that it can differentiate between when it needs to forward to the master vs. directly to the framework. There's also a bunch of code in the master that uses '`framework-pid`' that we'll need to go over and update to handle http schedulers. (2) To continue to support the scheduler driver's message passing optimization while still having a driver that speaks HTTP, we'll need to pass it's PID somehow. Vinod and I were thinking of having a special HTTP header to pass it through. I left some other comments, but let's figure out a plan during the http sync tomorrow. src/common/protobuf_utils.hpp (line 80) https://reviews.apache.org/r/36318/#comment146423 We usually add the argument name in the header, so: ``` Tryscheduler::Event event(const FrameworkRegisteredMessage message); ``` src/common/protobuf_utils.hpp (line 83) https://reviews.apache.org/r/36318/#comment146422 Why define this? Seems that we would want this case to be a compilation error instead of having an Error at runtime. src/common/protobuf_utils.cpp (lines 57 - 60) https://reviews.apache.org/r/36318/#comment146424 Mind fixing this in a separate patch since it's independent? We can get it committed quickly that way. Also, seems like we should have had message as an Option, but let's save that for later. src/common/protobuf_utils.cpp (lines 193 - 202) https://reviews.apache.org/r/36318/#comment146425 Why is this a Try? It seems pretty dangerous for message conversion to fail at run time, was there some errors we need to capture, even if we remove the generic 'Message' parsing error here (move it to compile time error). src/master/http.cpp (lines 314 - 325) https://reviews.apache.org/r/36318/#comment146450 Whoops, this will crash if contentType is none. Mind also being explicit about protobuf, rather than assuming that !json implies protobuf? Also, would you mind pulling out this code into a separate patch? It will make it easier to land incrementally :) src/master/http.cpp (line 346) https://reviews.apache.org/r/36318/#comment146451 Not yours, but a 501 seems more appropriate here for now? src/master/master.cpp (line 1747) https://reviews.apache.org/r/36318/#comment146452 frameworkInfo.name() is not equivalent to the framework's pid. Per our chat on IRC, we won't have the PID for pure HTTP frameworks, which means that the slave needs to send messages through the master, and we'll probably need to re-work some more of the master code as well. - Ben Mahler 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
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
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
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/#review92161 --- Ship it! Ship It! - Marco Massenzio 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
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
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
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
--- 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. Changes --- Simplified the design as per benh's comments, now the FrameworkDriver class just has an optional field pipe for http frameworks. Bugs: MESOS-2294 https://issues.apache.org/jira/browse/MESOS-2294 Repository: mesos Description (updated) --- 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 (updated) - 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
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
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
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
--- 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. Changes --- Addressed review comments + added a fix to close the pipe upon exit. Would add a test-case for it in a separate review when submitting the re-registration(...) change thereby testing for failover etc. 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 (updated) - 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
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
On July 8, 2015, 6:07 p.m., Isabel Jimenez wrote: src/master/http.cpp, line 316 https://reviews.apache.org/r/36318/diff/1/?file=1002351#file1002351line316 Al the json logic will be handled in the split of 36037 this lines aren't very useful here Left the TODO for now as a place-holder but removed the parsing logic. Not exactly the way you wanted it, but I am expecting this to go away anyways soon. On July 8, 2015, 6:07 p.m., Isabel Jimenez wrote: src/master/master.hpp, line 1584 https://reviews.apache.org/r/36318/diff/1/?file=1002352#file1002352line1584 We should leave this empty line My bad. - Anand --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/#review90946 --- 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
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/#review91169 --- Patch looks great! Reviews applied: [36318] All tests passed. - Mesos ReviewBot 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
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
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.
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).
- Marco
---
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
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36318/#review91213
---
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
src/common/protobuf_utils.hpp (lines 78 - 79)
https://reviews.apache.org/r/36318/#comment144492
please use javadoc format
also unnecessary semicolon
s/a/an
(eg an Event)
src/common/protobuf_utils.cpp (line 198)
https://reviews.apache.org/r/36318/#comment144493
an event
also the indent looks off to me?
(either four spaces or line up with quotes - but do you really need to
break the string?)
src/master/http.cpp (line 307)
https://reviews.apache.org/r/36318/#comment144494
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 :)
src/master/http.cpp (lines 311 - 316)
https://reviews.apache.org/r/36318/#comment144497
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 {
...
}
```
src/master/http.cpp (line 317)
https://reviews.apache.org/r/36318/#comment144495
the error is actually 415 Media Not Supported (I think - please double
check)
src/master/http.cpp (line 342)
https://reviews.apache.org/r/36318/#comment144498
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 :)
src/master/http.cpp (lines 1246 - 1249)
https://reviews.apache.org/r/36318/#comment144500
can you please avoid the leading underscore? they seem largely unnecessary
now that we have the trailing ones for the private members
src/master/http.cpp (line 1261)
https://reviews.apache.org/r/36318/#comment144501
no leading underscore
please add doxy for method
src/master/http.cpp (lines 1271 - 1273)
https://reviews.apache.org/r/36318/#comment144502
align (see other code)
src/master/http.cpp (lines 1277 - 1278)
https://reviews.apache.org/r/36318/#comment144503
here too
src/master/master.hpp (line 353)
https://reviews.apache.org/r/36318/#comment144504
use javadoc instead
- Marco Massenzio
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
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
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) Same here - Isabel --- 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
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
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 {
...
}
```
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.
- Isabel
---
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
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/#review91230 --- src/master/http.cpp (line 342) https://reviews.apache.org/r/36318/#comment144508 Same here. - Isabel Jimenez 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
Re: Review Request 36318: [MESOS-2294] Add support to master for streaming subscribed events
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,
