Re: Review Request 37097: Fix 'Accept-Encoding' parsing
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37097/#review94958 --- Ship it! Couple of comments below, but I'll make the adjustments and get this committed for you shortly! Thanks for splitting this out and updating the test! 3rdparty/libprocess/include/process/http.hpp (line 120) https://reviews.apache.org/r/37097/#comment149686 We're still wrapping comments at 70 for now, but that might change soon :) 3rdparty/libprocess/include/process/http.hpp (line 122) https://reviews.apache.org/r/37097/#comment149684 Which RFC? :) 3rdparty/libprocess/include/process/http.hpp (lines 124 - 125) https://reviews.apache.org/r/37097/#comment149683 This bit seems to just be re-iterating the summary above? 3rdparty/libprocess/src/http.cpp (lines 134 - 135) https://reviews.apache.org/r/37097/#comment149692 Reading through the RFC again, it's quite a bit more subtle than this: ``` 4. The identity content-coding is always acceptable, unless specifically refused because the Accept-Encoding field includes identity;q=0, or because the field includes *;q=0 and does not explicitly include the identity content-coding. If the Accept-Encoding field-value is empty, then only the identity encoding is acceptable. If no Accept-Encoding field is present in a request, the server MAY assume that the client will accept any content coding. In this case, if identity is one of the available content-codings, then the server SHOULD use the identity content-coding, unless it has additional information that a different content-coding is meaningful to the client. Note: If the request does not include an Accept-Encoding field, and if the identity content-coding is unavailable, then content-codings commonly understood by HTTP/1.0 clients (i.e., gzip and compress) are preferred; some older clients improperly display messages sent with other content-codings. The server might also make this decision based on information about the particular user-agent or client. Note: Most HTTP/1.0 applications do not recognize or obey qvalues associated with content-codings. This means that qvalues will not work and are not permitted with x-gzip or x-compress. ``` So it appears that we're doing the right thing here by returning false and using the identity encoding, but we don't correctly deal with explicitly rejected identity encoding for now.. I'll remove this and the TODO since it's a bit misleading 3rdparty/libprocess/src/http.cpp (lines 145 - 147) https://reviews.apache.org/r/37097/#comment149688 Shouldn't this be up where we're returning false..? 3rdparty/libprocess/src/http.cpp (line 159) https://reviews.apache.org/r/37097/#comment149701 Hm.. it's a bit implicit that tokens.empty is guaranteed to be false here because 'encoding' itself is non-empty due to it coming from tokenize. - Ben Mahler On Aug. 7, 2015, 6:52 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37097/ --- (Updated Aug. 7, 2015, 6:52 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- Currently parsing only compares the begining of the header making 'gzipbug' match with candidate 'gzip' Diffs - 3rdparty/libprocess/include/process/http.hpp b8d9300 3rdparty/libprocess/src/http.cpp 4dcbd74 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137 Diff: https://reviews.apache.org/r/37097/diff/ Testing --- make check Thanks, Isabel Jimenez
Re: Review Request 37097: Fix 'Accept-Encoding' parsing
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37097/#review94513 --- Ship it! 3rdparty/libprocess/src/http.cpp (line 133) https://reviews.apache.org/r/37097/#comment149139 s/2606/2616/ - Alexander Rojas On Aug. 6, 2015, 7:35 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37097/ --- (Updated Aug. 6, 2015, 7:35 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- Currently parsing only compares the begining of the header making 'gzipbug' match with candidate 'gzip' Diffs - 3rdparty/libprocess/include/process/http.hpp b8d9300 3rdparty/libprocess/src/http.cpp 4dcbd74 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137 Diff: https://reviews.apache.org/r/37097/diff/ Testing --- make check Thanks, Isabel Jimenez
Re: Review Request 37097: Fix 'Accept-Encoding' parsing
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37097/ --- (Updated Aug. 7, 2015, 6:52 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- Currently parsing only compares the begining of the header making 'gzipbug' match with candidate 'gzip' Diffs (updated) - 3rdparty/libprocess/include/process/http.hpp b8d9300 3rdparty/libprocess/src/http.cpp 4dcbd74 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137 Diff: https://reviews.apache.org/r/37097/diff/ Testing --- make check Thanks, Isabel Jimenez
Re: Review Request 37097: Fix 'Accept-Encoding' parsing
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37097/#review94386 --- 3rdparty/libprocess/src/http.cpp (lines 133 - 138) https://reviews.apache.org/r/37097/#comment148962 Can you leave the RFC number here? I find it is a good practice even if it is repeatedly used. 3rdparty/libprocess/src/http.cpp (lines 134 - 138) https://reviews.apache.org/r/37097/#comment148963 This paragraph doesn't seem to come from the RFC. Can you move it before the headline or after the contents from the rfc? 3rdparty/libprocess/src/http.cpp (line 159) https://reviews.apache.org/r/37097/#comment148964 I don't think we use `cout` in libprocess but `VLOG` (check `decode` in this file). - Alexander Rojas On Aug. 5, 2015, 9 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37097/ --- (Updated Aug. 5, 2015, 9 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- Currently parsing only compares the begining of the header making 'gzipbug' match with candidate 'gzip' Diffs - 3rdparty/libprocess/include/process/http.hpp b8d9300 3rdparty/libprocess/src/http.cpp 4dcbd74 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137 Diff: https://reviews.apache.org/r/37097/diff/ Testing --- make check Thanks, Isabel Jimenez
Re: Review Request 37097: Fix 'Accept-Encoding' parsing
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37097/ --- (Updated Aug. 6, 2015, 5:35 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- Currently parsing only compares the begining of the header making 'gzipbug' match with candidate 'gzip' Diffs (updated) - 3rdparty/libprocess/include/process/http.hpp b8d9300 3rdparty/libprocess/src/http.cpp 4dcbd74 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137 Diff: https://reviews.apache.org/r/37097/diff/ Testing --- make check Thanks, Isabel Jimenez
Re: Review Request 37097: Fix 'Accept-Encoding' parsing
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37097/ --- (Updated Aug. 5, 2015, 7 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- Currently parsing only compares the begining of the header making 'gzipbug' match with candidate 'gzip' Diffs (updated) - 3rdparty/libprocess/include/process/http.hpp b8d9300 3rdparty/libprocess/src/http.cpp 4dcbd74 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137 Diff: https://reviews.apache.org/r/37097/diff/ Testing --- make check Thanks, Isabel Jimenez
Re: Review Request 37097: Fix 'Accept-Encoding' parsing
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37097/#review94161 --- 3rdparty/libprocess/src/http.cpp https://reviews.apache.org/r/37097/#comment148696 What is the rationale behind moving these comments to the header file ? I wasn't able to make it out from r36402, Pardon my ignorance. Having these rules in the cpp file is much more intuitive since it allows you to easily understand the code as each rule is followed by its implementation. Including the rules in the header file is essentially duplicating the RFC. Why do we want to do it ? 3rdparty/libprocess/src/tests/encoder_tests.cpp (line 68) https://reviews.apache.org/r/37097/#comment148695 Can we kill the extra [0] index element we just introduced ? - Anand Mazumdar On Aug. 4, 2015, 10:43 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37097/ --- (Updated Aug. 4, 2015, 10:43 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- Currently parsing only compares the begining of the header making 'gzipbug' match with candidate 'gzip' Diffs - 3rdparty/libprocess/include/process/http.hpp b8d9300 3rdparty/libprocess/src/http.cpp 4dcbd74 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137 Diff: https://reviews.apache.org/r/37097/diff/ Testing --- make check Thanks, Isabel Jimenez
Re: Review Request 37097: Fix 'Accept-Encoding' parsing
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37097/ --- (Updated Aug. 4, 2015, 10:43 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- Currently parsing only compares the begining of the header making 'gzipbug' match with candidate 'gzip' Diffs (updated) - 3rdparty/libprocess/include/process/http.hpp b8d9300 3rdparty/libprocess/src/http.cpp 4dcbd74 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137 Diff: https://reviews.apache.org/r/37097/diff/ Testing --- make check Thanks, Isabel Jimenez
Re: Review Request 37097: Fix 'Accept-Encoding' parsing
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37097/ --- (Updated Aug. 4, 2015, 9:29 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- Currently parsing only compares the begining of the header making 'gzipbug' match with candidate 'gzip' Diffs (updated) - 3rdparty/libprocess/include/process/http.hpp b8d9300 3rdparty/libprocess/src/http.cpp 4dcbd74 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137 Diff: https://reviews.apache.org/r/37097/diff/ Testing --- make check Thanks, Isabel Jimenez
