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

Reply via email to