> On Jan. 3, 2013, 7:29 p.m., Vinod Kone wrote: > > > > Ben Mahler wrote: > So I'm holding off on updating this review until I figure out whether > I'll expand the Request / Response structs to have first class members for > each header.
I've moved the acceptable() function into the http namespace. > On Jan. 3, 2013, 7:29 p.m., Vinod Kone wrote: > > third_party/libprocess/src/encoder.hpp, line 240 > > <https://reviews.apache.org/r/8797/diff/1/?file=244240#file244240line240> > > > > when is "values["q"].size() != 1" possible? Although strings::pairs will always return values["q"].size() >= 1, I'd like to enforce that it's exactly 1. This means if they've done something like gzip;q=0;q=1, I'll just ignore their q value because it's bogus. > On Jan. 3, 2013, 7:29 p.m., Vinod Kone wrote: > > third_party/libprocess/src/encoder.hpp, line 228 > > <https://reviews.apache.org/r/8797/diff/1/?file=244240#file244240line228> > > > > how is the * matched? >From my comment: // 2. The special "*" symbol in an Accept-Encoding field matches any // available content-coding not explicitly listed in the header // field. Rule 2 here refers to this rule: targets.push_back("*"); // Rule 2. Make sense? > On Jan. 3, 2013, 7:29 p.m., Vinod Kone wrote: > > third_party/libprocess/src/encoder.hpp, line 246 > > <https://reviews.apache.org/r/8797/diff/1/?file=244240#file244240line246> > > > > can the 'q' value be negative? No: http://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html#sec3.9 But there's nothing to stop a request from using a malformed q value. Why did you raise an issue here? > On Jan. 3, 2013, 7:29 p.m., Vinod Kone wrote: > > third_party/libprocess/src/encoder.hpp, line 216 > > <https://reviews.apache.org/r/8797/diff/1/?file=244240#file244240line216> > > > > it is confusing to use the variable 'encodings' for acceptable > > encodings, when the requested encoding is called 'encoding'. So I've renamed encodings -> acceptableEncodings. > On Jan. 3, 2013, 7:29 p.m., Vinod Kone wrote: > > third_party/libprocess/src/encoder.hpp, line 233 > > <https://reviews.apache.org/r/8797/diff/1/?file=244240#file244240line233> > > > > for eg, this would be easier to grok if you did > > > > foreach(std::string& encoding, encodings) This is now foreach (acceptable, acceptableEncodings) { .. } Is that more clear? > On Jan. 3, 2013, 7:29 p.m., Vinod Kone wrote: > > third_party/libprocess/src/process.cpp, line 320 > > <https://reviews.apache.org/r/8797/diff/1/?file=244241#file244241line320> > > > > From the above, it sounds like 'request' should contain the information > > about 'persist'? if so, isnt this redundant? Almost.. persist is a combination of the request and response. persist = request.keepAlive && ! (response has Connection: close) or when response is PIPE persist = true So since that is already figured out when this is called, I didn't want to do that logic in two places. The code in HttpProxy::process has this logic if you have cleanup ideas! - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8797/#review15043 ----------------------------------------------------------- On Jan. 18, 2013, 1:06 a.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8797/ > ----------------------------------------------------------- > > (Updated Jan. 18, 2013, 1:06 a.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 > 40248dab3df59b322ad9c5a4858961ef471779fd > third_party/libprocess/src/encoder.hpp > 9664f38b7b972f196921ef13adb8c9caa180ca65 > third_party/libprocess/src/process.cpp > 72e437862ee0b35126c16d32bec79ef76a4e2b23 > > Diff: https://reviews.apache.org/r/8797/diff/ > > > Testing > ------- > > make check (also added some tests in the following reviews). > > > Thanks, > > Ben Mahler > >
