You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Isabel Jimenez <co...@isabeljimenez.com> on 2015/06/30 11:06:50 UTC
Review Request 36040: Change Server closing connections for every
request
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36040/
-----------------------------------------------------------
Review request for mesos, Anand Mazumdar, Marco Massenzio, and Vinod Kone.
Repository: mesos-incubating
Description
-------
Adding the possibility to 'keep-alive' the connection
Diffs
-----
3rdparty/libprocess/src/http.cpp 0898335
Diff: https://reviews.apache.org/r/36040/diff/
Testing
-------
make check
Thanks,
Isabel Jimenez
Re: Review Request 36040: Change Server closing connections for every
request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
> On June 30, 2015, 4:24 p.m., Anand Mazumdar wrote:
> > 3rdparty/libprocess/src/http.cpp, line 777
> > <https://reviews.apache.org/r/36040/diff/1/?file=995839#file995839line777>
> >
> > What is this change intending to solve ? If it's the ability to not close the connection for streamed responses.
> >
> > We would only like to not close the connection i.e. communicate to the client that it's a persistent connection only when using the streaming API's.
> >
> > So the condition should be :
> >
> > if ( streamedResponse ) {
> > ... // don't send the close header
> > }
> >
> > In all other cases, we don't want to use the same connection again, and it's fine to send the "close" header like we are doing.
> >
> > Can you also add @bmahler as a reviewer to confirm this ?
>
> Isabel Jimenez wrote:
> I mostly add this for tests on 36037, this overrides the conenction header otherwise.
>
> Anand Mazumdar wrote:
> As per our discussion (bmahler/me/isabel) we should be able to discard this review for now and may be revisit this later ?
>
> The client can signal it's intent to "Keep-Alive" via the connection header, but the server can decide to honor it or now, in our case we just send the "Connection: Close" everytime, which is perfectly acceptable in my opinion.
IMO the best thing would be to keep the patch, I'd have to add an extended comment explaning that we don't support keep-alive in any case (for now, I should also add a TODO so we add support for this in libprocess), but that we shouldn't be overwritting requests headers on our code anyways. What do you think? @bmahler @anandmazumbar
- Isabel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36040/#review89909
-----------------------------------------------------------
On June 30, 2015, 9:06 a.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36040/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 9:06 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding the possibility to 'keep-alive' the connection
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/http.cpp 0898335
>
> Diff: https://reviews.apache.org/r/36040/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36040: Change Server closing connections for every
request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
> On June 30, 2015, 4:24 p.m., Anand Mazumdar wrote:
> > 3rdparty/libprocess/src/http.cpp, line 777
> > <https://reviews.apache.org/r/36040/diff/1/?file=995839#file995839line777>
> >
> > What is this change intending to solve ? If it's the ability to not close the connection for streamed responses.
> >
> > We would only like to not close the connection i.e. communicate to the client that it's a persistent connection only when using the streaming API's.
> >
> > So the condition should be :
> >
> > if ( streamedResponse ) {
> > ... // don't send the close header
> > }
> >
> > In all other cases, we don't want to use the same connection again, and it's fine to send the "close" header like we are doing.
> >
> > Can you also add @bmahler as a reviewer to confirm this ?
I mostly add this for tests on 36037, this overrides the conenction header otherwise.
- Isabel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36040/#review89909
-----------------------------------------------------------
On June 30, 2015, 9:06 a.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36040/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 9:06 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding the possibility to 'keep-alive' the connection
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/http.cpp 0898335
>
> Diff: https://reviews.apache.org/r/36040/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36040: Change Server closing connections for every
request
Posted by Anand Mazumdar <ma...@gmail.com>.
> On June 30, 2015, 4:24 p.m., Anand Mazumdar wrote:
> > 3rdparty/libprocess/src/http.cpp, line 777
> > <https://reviews.apache.org/r/36040/diff/1/?file=995839#file995839line777>
> >
> > What is this change intending to solve ? If it's the ability to not close the connection for streamed responses.
> >
> > We would only like to not close the connection i.e. communicate to the client that it's a persistent connection only when using the streaming API's.
> >
> > So the condition should be :
> >
> > if ( streamedResponse ) {
> > ... // don't send the close header
> > }
> >
> > In all other cases, we don't want to use the same connection again, and it's fine to send the "close" header like we are doing.
> >
> > Can you also add @bmahler as a reviewer to confirm this ?
>
> Isabel Jimenez wrote:
> I mostly add this for tests on 36037, this overrides the conenction header otherwise.
As per our discussion (bmahler/me/isabel) we should be able to discard this review for now and may be revisit this later ?
The client can signal it's intent to "Keep-Alive" via the connection header, but the server can decide to honor it or now, in our case we just send the "Connection: Close" everytime, which is perfectly acceptable in my opinion.
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36040/#review89909
-----------------------------------------------------------
On June 30, 2015, 9:06 a.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36040/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 9:06 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding the possibility to 'keep-alive' the connection
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/http.cpp 0898335
>
> Diff: https://reviews.apache.org/r/36040/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36040: Change Server closing connections for every
request
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36040/#review89909
-----------------------------------------------------------
3rdparty/libprocess/src/http.cpp (line 777)
<https://reviews.apache.org/r/36040/#comment142786>
What is this change intending to solve ? If it's the ability to not close the connection for streamed responses.
We would only like to not close the connection i.e. communicate to the client that it's a persistent connection only when using the streaming API's.
So the condition should be :
if ( streamedResponse ) {
... // don't send the close header
}
In all other cases, we don't want to use the same connection again, and it's fine to send the "close" header like we are doing.
Can you also add @bmahler as a reviewer to confirm this ?
- Anand Mazumdar
On June 30, 2015, 9:06 a.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36040/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 9:06 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding the possibility to 'keep-alive' the connection
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/http.cpp 0898335
>
> Diff: https://reviews.apache.org/r/36040/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36040: Change Server closing connections for every
request
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36040/#review90187
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/src/http.cpp (lines 778 - 780)
<https://reviews.apache.org/r/36040/#comment143185>
I have the impression your change introduced tabs?
(may be an RB artifact, though)
can you please add `is` (header *is* not...)
- Marco Massenzio
On June 30, 2015, 9:06 a.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36040/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 9:06 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Adding the possibility to 'keep-alive' the connection
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/http.cpp 0898335
>
> Diff: https://reviews.apache.org/r/36040/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 36040: Change Server closing connections for every
request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36040/
-----------------------------------------------------------
(Updated July 2, 2015, 10:32 p.m.)
Review request for mesos, Anand Mazumdar, Marco Massenzio, and Vinod Kone.
Repository: mesos-incubating
Description (updated)
-------
Adding a TODO so later we have the possibility to 'keep-alive' the connection
Diffs
-----
3rdparty/libprocess/src/http.cpp 0898335
Diff: https://reviews.apache.org/r/36040/diff/
Testing
-------
make check
Thanks,
Isabel Jimenez
Re: Review Request 36040: Change Server closing connections for every
request
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36040/
-----------------------------------------------------------
(Updated July 2, 2015, 10:32 p.m.)
Review request for mesos, Anand Mazumdar, Marco Massenzio, and Vinod Kone.
Repository: mesos-incubating
Description
-------
Adding the possibility to 'keep-alive' the connection
Diffs (updated)
-----
3rdparty/libprocess/src/http.cpp 0898335
Diff: https://reviews.apache.org/r/36040/diff/
Testing
-------
make check
Thanks,
Isabel Jimenez