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

Reply via email to