You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2013/03/02 22:38:58 UTC
Re: Review Request: Use gzip encoding only when acceptable.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8797/
-----------------------------------------------------------
(Updated March 2, 2013, 9:38 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Rebased off trunk.
Description
-------
See bug description.
This addresses bug MESOS-309.
https://issues.apache.org/jira/browse/MESOS-309
Diffs (updated)
-----
third_party/libprocess/include/process/http.hpp bc397e6f1d539d621b85d7ab36afb0237ceefca9
third_party/libprocess/src/encoder.hpp 9664f38b7b972f196921ef13adb8c9caa180ca65
third_party/libprocess/src/process.cpp 8ae20b5d9ffc415e36e076ed78842aae7ddd4314
Diff: https://reviews.apache.org/r/8797/diff/
Testing
-------
make check (also added some tests in the following reviews).
Thanks,
Ben Mahler
Re: Review Request: Use gzip encoding only when acceptable.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8797/#review17431
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Hindman
On March 5, 2013, 10:18 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8797/
> -----------------------------------------------------------
>
> (Updated March 5, 2013, 10:18 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> See bug description.
>
>
> This addresses bug MESOS-309.
> https://issues.apache.org/jira/browse/MESOS-309
>
>
> Diffs
> -----
>
> third_party/libprocess/include/process/http.hpp bc397e6f1d539d621b85d7ab36afb0237ceefca9
> third_party/libprocess/src/encoder.hpp 9664f38b7b972f196921ef13adb8c9caa180ca65
> third_party/libprocess/src/process.cpp 8ae20b5d9ffc415e36e076ed78842aae7ddd4314
>
> Diff: https://reviews.apache.org/r/8797/diff/
>
>
> Testing
> -------
>
> make check (also added some tests in the following reviews).
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Use gzip encoding only when acceptable.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8797/
-----------------------------------------------------------
(Updated March 5, 2013, 10:18 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
benh review + rebased off trunk.
Description
-------
See bug description.
This addresses bug MESOS-309.
https://issues.apache.org/jira/browse/MESOS-309
Diffs (updated)
-----
third_party/libprocess/include/process/http.hpp bc397e6f1d539d621b85d7ab36afb0237ceefca9
third_party/libprocess/src/encoder.hpp 9664f38b7b972f196921ef13adb8c9caa180ca65
third_party/libprocess/src/process.cpp 8ae20b5d9ffc415e36e076ed78842aae7ddd4314
Diff: https://reviews.apache.org/r/8797/diff/
Testing
-------
make check (also added some tests in the following reviews).
Thanks,
Ben Mahler
Re: Review Request: Use gzip encoding only when acceptable.
Posted by Ben Mahler <be...@gmail.com>.
> On March 5, 2013, 9:46 a.m., Benjamin Hindman wrote:
> > third_party/libprocess/include/process/http.hpp, line 75
> > <https://reviews.apache.org/r/8797/diff/5/?file=264506#file264506line75>
> >
> > Again, just a naming thing, but I find 'strings::startsWith(encoding, candidate)' better as I read it aloud as "does encoding start with candidate?" which sounds quite natural. I understand that encoding is already used here, so feel free to ignore.
Yeah.. I noticed this as well but punted since encoding was already used, went with _encoding since I really like the readability of using "encoding".
> On March 5, 2013, 9:46 a.m., Benjamin Hindman wrote:
> > third_party/libprocess/src/process.cpp, line 1583
> > <https://reviews.apache.org/r/8797/diff/5/?file=264508#file264508line1583>
> >
> > See my comment below about not needing to pass the 'persist' parameter. For this case, I suggest creating an HttpResponseEncoder directly so that you can force the persistance of the socket. That is, it could be that the request doesn't have keep-alive, in fact, it could be that the request asked to close the connection after the data is returned. But since we're going to sendfile the data still we don't want to close the socket yet.
Ok, fixed. Please take a look!
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8797/#review17393
-----------------------------------------------------------
On March 2, 2013, 9:38 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8797/
> -----------------------------------------------------------
>
> (Updated March 2, 2013, 9:38 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> See bug description.
>
>
> This addresses bug MESOS-309.
> https://issues.apache.org/jira/browse/MESOS-309
>
>
> Diffs
> -----
>
> third_party/libprocess/include/process/http.hpp bc397e6f1d539d621b85d7ab36afb0237ceefca9
> third_party/libprocess/src/encoder.hpp 9664f38b7b972f196921ef13adb8c9caa180ca65
> third_party/libprocess/src/process.cpp 8ae20b5d9ffc415e36e076ed78842aae7ddd4314
>
> Diff: https://reviews.apache.org/r/8797/diff/
>
>
> Testing
> -------
>
> make check (also added some tests in the following reviews).
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Use gzip encoding only when acceptable.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8797/#review17393
-----------------------------------------------------------
Ship it!
third_party/libprocess/include/process/http.hpp
<https://reviews.apache.org/r/8797/#comment36905>
This can be a const&, and you can also kill it in favor of embedding the tokenize call directly in the foreach below.
third_party/libprocess/include/process/http.hpp
<https://reviews.apache.org/r/8797/#comment36903>
Maybe s/targets/candidates/? I see this as a list of candidates you want to check for acceptableness.
third_party/libprocess/include/process/http.hpp
<https://reviews.apache.org/r/8797/#comment36908>
Seems like this should go above the declaration of the vector above.
third_party/libprocess/include/process/http.hpp
<https://reviews.apache.org/r/8797/#comment36904>
This would then read as:
Is the candidate one of the accepted encodings?
third_party/libprocess/include/process/http.hpp
<https://reviews.apache.org/r/8797/#comment36906>
Again, just a naming thing, but I find 'strings::startsWith(encoding, candidate)' better as I read it aloud as "does encoding start with candidate?" which sounds quite natural. I understand that encoding is already used here, so feel free to ignore.
third_party/libprocess/include/process/http.hpp
<https://reviews.apache.org/r/8797/#comment36907>
Can const & here too.
third_party/libprocess/src/encoder.hpp
<https://reviews.apache.org/r/8797/#comment36909>
Reads so nicely!
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/8797/#comment36915>
See my comment below, but you can kill this.
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/8797/#comment36916>
See my comment below, but you can kill this logic here.
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/8797/#comment36914>
See my comment below about not needing to pass the 'persist' parameter. For this case, I suggest creating an HttpResponseEncoder directly so that you can force the persistance of the socket. That is, it could be that the request doesn't have keep-alive, in fact, it could be that the request asked to close the connection after the data is returned. But since we're going to sendfile the data still we don't want to close the socket yet.
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/8797/#comment36911>
Ditto here: "For this case, I suggest creating an HttpResponseEncoder directly so that you can force the persistance of the socket. That is, it could be that the request doesn't have keep-alive, in fact, it could be that the request asked to close the connection after the data is returned."
... but since we might need to write more data after this SocketManager::send call we don't want to close the socket yet.
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/8797/#comment36912>
This is actually a bug from my original code. We don't actually want to use 'request.keepAlive' until the very last SocketManger::send call. See the fix below.
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/8797/#comment36913>
s/persist/finished ? persist : true/
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/8797/#comment36910>
Persist seems redundant here. You now have all the information you need to decide whether or not to persist:
============
bool persist = request.keepAlive;
// Don't persist connection if headers include 'Connection: close'.
if (response.headers.count("Connection") > 0) {
const string& connection = response.headers.find("Connection")->second;
if (connection == "close") {
persist = false;
}
}
============
And then you can kill that code in HttpProxy::process too.
- Benjamin Hindman
On March 2, 2013, 9:38 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8797/
> -----------------------------------------------------------
>
> (Updated March 2, 2013, 9:38 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> See bug description.
>
>
> This addresses bug MESOS-309.
> https://issues.apache.org/jira/browse/MESOS-309
>
>
> Diffs
> -----
>
> third_party/libprocess/include/process/http.hpp bc397e6f1d539d621b85d7ab36afb0237ceefca9
> third_party/libprocess/src/encoder.hpp 9664f38b7b972f196921ef13adb8c9caa180ca65
> third_party/libprocess/src/process.cpp 8ae20b5d9ffc415e36e076ed78842aae7ddd4314
>
> Diff: https://reviews.apache.org/r/8797/diff/
>
>
> Testing
> -------
>
> make check (also added some tests in the following reviews).
>
>
> Thanks,
>
> Ben Mahler
>
>