mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 38608: Introduced an http::Connection for connection re-use and pipelining.
Date Thu, 01 Oct 2015 00:19:49 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message