----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38608/#review100717 -----------------------------------------------------------
3rdparty/libprocess/include/process/http.hpp (line 760) <https://reviews.apache.org/r/38608/#comment157976> Can you add a comment here that this is forward declared? Stumped me for a bit. 3rdparty/libprocess/src/http.cpp (line 761) <https://reviews.apache.org/r/38608/#comment157979> s/s/socket/ ? 3rdparty/libprocess/src/http.cpp (line 783) <https://reviews.apache.org/r/38608/#comment157980> ditto. s/s/socket/ 3rdparty/libprocess/src/http.cpp (line 791) <https://reviews.apache.org/r/38608/#comment158358> why the temporary? can't you just do "return promise.future()"? 3rdparty/libprocess/src/http.cpp (line 819) <https://reviews.apache.org/r/38608/#comment157981> s/, we/. We/ 3rdparty/libprocess/src/http.cpp (line 832) <https://reviews.apache.org/r/38608/#comment158371> 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. 3rdparty/libprocess/src/http.cpp (line 886) <https://reviews.apache.org/r/38608/#comment157985> Can you add a comment on when this is possible, considering the above if/else check? 3rdparty/libprocess/src/http.cpp (line 891) <https://reviews.apache.org/r/38608/#comment158360> per line #915 this could also happen if connection close is received while there are pending requests right? 3rdparty/libprocess/src/http.cpp (lines 901 - 905) <https://reviews.apache.org/r/38608/#comment158359> What's happening here? Can you add a comment? 3rdparty/libprocess/src/http.cpp (line 952) <https://reviews.apache.org/r/38608/#comment157983> s/s/socket/ 3rdparty/libprocess/src/http.cpp (line 955) <https://reviews.apache.org/r/38608/#comment157984> Add a comment on what the tuple represents? esp. the streaming response part. 3rdparty/libprocess/src/http.cpp (line 960) <https://reviews.apache.org/r/38608/#comment157982> s/closeConnection/close/ ? 3rdparty/libprocess/src/http.cpp (lines 969 - 971) <https://reviews.apache.org/r/38608/#comment158361> lets pull process below to spawn, like we do in most of our code base? 3rdparty/libprocess/src/http.cpp (line 976) <https://reviews.apache.org/r/38608/#comment158362> a comment on why you need 'false' here? 3rdparty/libprocess/src/http.cpp (lines 1021 - 1042) <https://reviews.apache.org/r/38608/#comment157977> Seems generally useful. Would it make sense to make this a factory method on Address class? Try<Address> create(const URL& url); 3rdparty/libprocess/src/http.cpp (lines 1045 - 1059) <https://reviews.apache.org/r/38608/#comment157978> this seems like a repeated pattern in this file. can this be a factory method on Socket? 3rdparty/libprocess/src/tests/http_tests.cpp (line 672) <https://reviews.apache.org/r/38608/#comment158364> put Return on next line. 3rdparty/libprocess/src/tests/http_tests.cpp (line 689) <https://reviews.apache.org/r/38608/#comment158366> s/request/response/ ? - Vinod Kone 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 > >
