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

Reply via email to