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: Refactored framework struct in master to support http frameworks
Date Thu, 23 Jul 2015 04:53:40 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.
> 
> Anand Mazumdar wrote:
>     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.

Addressed (1) by refactoring the framework struct to make pid an optional field.


> 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.
> 
> Anand Mazumdar wrote:
>     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.

Dropping this as per my earlier comment. This method would anyways go away once we have implemented
all message types.


> 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);
> >     ```
> 
> Anand Mazumdar wrote:
>     Will do. thanks for pointing out.

Fixed.


> 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.
> 
> Anand Mazumdar wrote:
>     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 ?

Moved to patch 36717.


> 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).
> 
> Anand Mazumdar wrote:
>     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 ?

Added a TODO that we can change the return type to just Event when we have implemented all
message types.


> 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 :)
> 
> Anand Mazumdar wrote:
>     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  ?

Moved this to separate patch now to take care of this. r36720


> 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.
> 
> Anand Mazumdar wrote:
>     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 ?

Fixed.


> 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?
> 
> Anand Mazumdar wrote:
>     Sure, I can make the change.

CR: https://reviews.apache.org/r/36720/


- Anand


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


On July 23, 2015, 4:25 a.m., Anand Mazumdar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36318/
> -----------------------------------------------------------
> 
> (Updated July 23, 2015, 4:25 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 refactors the framework struct in master to introduce support for
> http frameworks.
> - pid becomes a optional field now in the framework struct.
> - added optional fields for supporting http frameworks to the framework struct
> - added a subcribe(...) method that
>   registerFramework(...)/subscribeHttpFramework(...) call into. Similar
>   functionality needs to be added for reregister to call into subscribe too.
> 
> Apologies for the extended diff ( I moved the framework struct to the end of
> the file as it used some functionality from the master class. The method was
> templated and had to implemented in the header file )
> 
> 
> Diffs
> -----
> 
>   src/common/protobuf_utils.hpp 2e827a0923de83d5cf853a12435b451cc7c55891 
>   src/common/protobuf_utils.cpp e0f82b53f5e106bbf4e21d6ac946df0fae821882 
>   src/master/http.cpp 6f5ca02c52462495b84e77525a6c88299746ece2 
>   src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec 
>   src/master/master.cpp 2f00f240ed2cd59ec0c2eae7fd2567f0edb8d9e0 
> 
> Diff: https://reviews.apache.org/r/36318/diff/
> 
> 
> Testing
> -------
> 
> make check + tests now go in a separate patch now.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>


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