> 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 Try<Event> 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=1012222#file1012222line1747>
> >
> >     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 ready and 
allowing us to implement more calls and a way for us to send events on the 
stream. What do you think ?


> On July 21, 2015, 12:58 a.m., Ben Mahler wrote:
> > src/common/protobuf_utils.hpp, line 80
> > <https://reviews.apache.org/r/36318/diff/3/?file=1012218#file1012218line80>
> >
> >     We usually add the argument name in the header, so:
> >     
> >     ```
> >     Try<scheduler::Event> event(const FrameworkRegisteredMessage& message);
> >     ```

Will do. thanks for pointing out.


- Anand


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


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