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