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