-----------------------------------------------------------
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:
    
    ```
    Try<scheduler::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 for subscribe call received a subscribed event 
> back on the stream.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Reply via email to