Re: 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/#review95498 --- This has not been touched in more than a month. Should it be discarded? - Marco Massenzio On July 2, 2015, 10:33 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36040/ --- (Updated July 2, 2015, 10:33 p.m.) Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone. Repository: mesos-incubating Description --- 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
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
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
--- 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
--- 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
Re: 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/#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
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
---
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
