> On Sept. 30, 2015, 1:32 a.m., Vinod Kone wrote: > >
Thanks! I've simplified things and also caught two issues that were missed originally. > On Sept. 30, 2015, 1:32 a.m., Vinod Kone wrote: > > 3rdparty/libprocess/src/http.cpp, lines 1045-1059 > > <https://reviews.apache.org/r/38608/diff/1/?file=1079766#file1079766line1045> > > > > this seems like a repeated pattern in this file. can this be a factory > > method on Socket? It's only repeated because I split out https://reviews.apache.org/r/38609/ . After applying the subsequent review it's not repeated. Agreed that we will want to see if this warrants being pulled out though if we end up with duplication. > On Sept. 30, 2015, 1:32 a.m., Vinod Kone wrote: > > 3rdparty/libprocess/src/http.cpp, lines 1021-1042 > > <https://reviews.apache.org/r/38608/diff/1/?file=1079766#file1079766line1021> > > > > Seems generally useful. Would it make sense to make this a factory > > method on Address class? > > > > Try<Address> create(const URL& url); Yeah, although it seems a bit odd for Address to understand URL since we may have arbitrary things that can get converted to addresses. One suggestion is to add this on URL: ``` class URL { Try<Address> resolve(); // Or, Try<Address> address(); }; ``` Added a TODO for follow-up. > On Sept. 30, 2015, 1:32 a.m., Vinod Kone wrote: > > 3rdparty/libprocess/src/http.cpp, line 976 > > <https://reviews.apache.org/r/38608/diff/1/?file=1079766#file1079766line976> > > > > a comment on why you need 'false' here? Thanks for the poke, this definitely warranted a comment! > On Sept. 30, 2015, 1:32 a.m., Vinod Kone wrote: > > 3rdparty/libprocess/src/http.cpp, lines 969-971 > > <https://reviews.apache.org/r/38608/diff/1/?file=1079766#file1079766line969> > > > > lets pull process below to spawn, like we do in most of our code base? Doing a grep reveals that we do this both ways, but I'll change it since your suggestion seems a bit more readable. > On Sept. 30, 2015, 1:32 a.m., Vinod Kone wrote: > > 3rdparty/libprocess/src/http.cpp, line 791 > > <https://reviews.apache.org/r/38608/diff/1/?file=1079766#file1079766line791> > > > > why the temporary? > > > > can't you just do "return promise.future()"? The promise is moved into the queue, so the return would have to be: ``` return std::get<1>(pipeline.back()).future(); ``` > On Sept. 30, 2015, 1:32 a.m., Vinod Kone wrote: > > 3rdparty/libprocess/src/http.cpp, line 832 > > <https://reviews.apache.org/r/38608/diff/1/?file=1079766#file1079766line832> > > > > the whole disconnect(), closed() and fail() semantics are a bit hard to > > grok, esp around what calls what. can you think of a refactor that will > > make it easy? sorry, i don't have concrete suggestion yet. Thanks for calling this out! This was definitely a difficulty when writing this. I've now greatly simplified this by having just a single 'disconnected' method. I hope this reads a lot clearer now, have a look! - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38608/#review100717 ----------------------------------------------------------- On Sept. 22, 2015, 6:18 a.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38608/ > ----------------------------------------------------------- > > (Updated Sept. 22, 2015, 6:18 a.m.) > > > Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone. > > > Bugs: MESOS-3332 > https://issues.apache.org/jira/browse/MESOS-3332 > > > Repository: mesos > > > Description > ------- > > In order to support connection re-use and pipelining of requests on a shared > connection, this introduces the notion of an http::Connection. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/http.hpp > fbd6cf7967173495875a8ea90ed28ade88b982a2 > 3rdparty/libprocess/src/http.cpp 9ad613a16c379b6d76a9a0f8d6160fe23a182fd4 > 3rdparty/libprocess/src/tests/http_tests.cpp > d0b9399d38fa284466a012a21080b1d9007af98b > > Diff: https://reviews.apache.org/r/38608/diff/ > > > Testing > ------- > > Added tests. > > > Thanks, > > Ben Mahler > >
