> On Aug. 12, 2015, 5:09 a.m., Ben Mahler wrote: > > Sorry for not elaborating on all of these, I added some more explanations > > here. Main thing is cleaning up the read loop and figuring out the callback > > semantics (do we need to revisit 'connected' / 'disconnected'?).
Let's keep the callback behavior the same as what it was before. We can decide to change the semantics if we feel the need later in a separate patch. > On Aug. 12, 2015, 5:09 a.m., Ben Mahler wrote: > > src/scheduler/scheduler.cpp, line 117 > > <https://reviews.apache.org/r/37303/diff/4/?file=1038063#file1038063line117> > > > > Sorry, let me elaborate further. I mentioned not having this because > > "headers" requires non-local reasoning when reading the request sending > > code: > > > > ``` > > // Send a streaming request for Subscribe call. > > response = process::http::streaming::post( > > master.get(), > > "api/v1/scheduler", > > headers, // non-local > > body, > > stringify(contentType)); > > > > vs. > > > > // Send a streaming request for Subscribe call. > > response = process::http::streaming::post( > > master.get(), > > "api/v1/scheduler", > > {{"Accept", stringify(contentType)}}, // local > > body, > > stringify(contentType)); > > ``` > > > > We've generally found local reasoning makes code easier to read (a.k.a. > > "readable"). Also, keep in mind that in the future you'll be able to send a > > Request directly, so it would become: > > > > ``` > > Request request; > > ... > > request.headers["Accept"] = stringify(contentType); > > > > response = process::http::streaming::request(request); > > ``` > > > > The other concern is that headers is not necessarily going to remain > > constant like this in the future (e.g. if we add the notion of a session > > header). Make sense? Anyways , this won't compile. I added it as a const local variable for now. > On Aug. 12, 2015, 5:09 a.m., Ben Mahler wrote: > > src/common/http.hpp, lines 56-57 > > <https://reviews.apache.org/r/37303/diff/4/?file=1038062#file1038062line56> > > > > You can `return stream << ...;` in a single statement. Looked more readable this way. Made the change though. > On Aug. 12, 2015, 5:09 a.m., Ben Mahler wrote: > > src/scheduler/scheduler.cpp, line 329 > > <https://reviews.apache.org/r/37303/diff/4/?file=1038063#file1038063line329> > > > > The reason I ask to flip this is we've generally not used "yoda" style > > comparisons, e.g. > > > > ``` > > size() > 1 > > > > rather than > > > > 1 < size() > > ``` > > > > Can you do a sweep in the code you've introduced here? Fixed. I guess we already have -Wall,-Werror in our code. So my concerns around accidently assigning in "if" statements don't make much sense here. > On Aug. 12, 2015, 5:09 a.m., Ben Mahler wrote: > > src/scheduler/scheduler.cpp, line 330 > > <https://reviews.apache.org/r/37303/diff/4/?file=1038063#file1038063line330> > > > > Flip the comparison here as well. Be sure to do a sweep. Done. CHECK_EQ though normally have syntax like CHECK_EQ(expected, actual) > On Aug. 12, 2015, 5:09 a.m., Ben Mahler wrote: > > src/scheduler/scheduler.cpp, lines 354-362 > > <https://reviews.apache.org/r/37303/diff/4/?file=1038063#file1038063line354> > > > > I suggested the struct because passing around the recodio reader > > independently of the pipe reader seems to be not that intuitive, was hoping > > to see them stored together inside an Option<Connection> rather than having > > the Option<Pipe::Reader> and recordio::Reader stored separately. Make more > > sense? > > > > Also, can we use recordio::Reader to be more explicit about this type? Done, Moved it to a struct Connection. Also since RecordIO::Reader is a process now. Saving it in a process::Owned member field inside the struct. > On Aug. 12, 2015, 5:09 a.m., Ben Mahler wrote: > > src/scheduler/scheduler.cpp, lines 387-394 > > <https://reviews.apache.org/r/37303/diff/4/?file=1038063#file1038063line387> > > > > Ok, let's chat about this what to do here, might need to revisit the > > callbacks here now that we have http. But this shouldn't be an error since > > it's just disconnection. If we call disconnected here though, seems that we > > need to immediately call connected if there is still a master available, > > otherwise we might hang around when we should be retrying. Removed the error event. Just logging an error for now. > On Aug. 12, 2015, 5:09 a.m., Ben Mahler wrote: > > src/scheduler/scheduler.cpp, lines 388-389 > > <https://reviews.apache.org/r/37303/diff/4/?file=1038063#file1038063line388> > > > > There is no "end of file chunk".. :) Fixed, My bad :) Even end of file event hardly made any sense , made it "End-Of-File received from master. The master closed the event stream" - Anand ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37303/#review95059 ----------------------------------------------------------- On Aug. 12, 2015, 6:51 a.m., Anand Mazumdar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37303/ > ----------------------------------------------------------- > > (Updated Aug. 12, 2015, 6:51 a.m.) > > > Review request for mesos, Ben Mahler and Vinod Kone. > > > Bugs: MESOS-2552 > https://issues.apache.org/jira/browse/MESOS-2552 > > > Repository: mesos > > > Description > ------- > > This changes moves scheduler library to http. The change to remove auth + > install,receive handlers are in other reviews. > > > Diffs > ----- > > src/common/http.hpp 98a12709ae5fcbf4ce3e37d164bff43818096838 > src/scheduler/scheduler.cpp 3fbe3831cf93c29180bc7e433fd57c6108988316 > > Diff: https://reviews.apache.org/r/37303/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Anand Mazumdar > >
