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 Tue, 21 Jul 2015 01:31:12 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message