Conversation
httpcache.go
Outdated
There was a problem hiding this comment.
Use CamelCase for consistency with the stdlib (see http://www.reddit.com/r/golang/comments/2az1fz/convention_for_nonexported_constans/ for a discussion of this).
|
updated by renaming constants |
httpcache.go
Outdated
There was a problem hiding this comment.
Why did you choose to omit the error checks?
There was a problem hiding this comment.
for the same reason i left the out the checks at line 81: if the request is broken it should get as it is to the server so that it might return a 400 or a relevant error, it's not the job of a caching proxy to validate these errors. the problem would be on the client sending a broken request and the error needs to bubble up on the server so that it can get fixed. just my opinion of course, it can be changed. what to do in that case though? removing the range header and letting the request through sounds wrong, giving an error before the server does too..suggestions?
|
httpcache.go
Outdated
There was a problem hiding this comment.
It's not necessary (for our needs) and probably not as useful to support multiple byte range requests (e.g., specifying multiple ranges with comma separators; see http://greenbytes.de/tech/webdav/draft-ietf-httpbis-p5-range-latest.html#rule.ranges-specifier), but it should not produce incorrect behavior in that case. (Currently it would fail to parse the ints because they would include commas, and then it would return a 0-range response, I believe.)
There was a problem hiding this comment.
yep, forgot to handle the comma cases, sorry about that
Clean up logging and avoid fmt
…filling the first one Added check for rangeStart to be less than rangeEnd
Added validateRanges to handle the case we have a subrange of an already existing query
|
New version that addresses your comments, i also have the PR for removing gocheck but i will make it wait until i can merge it with this one |
|
any news on this? |
httpcache.go
Outdated
There was a problem hiding this comment.
Compile this regexp in init to avoid runtime perf hit
|
Is the Content-Range header being set on responses to indicate which byte ranges were actually fulfilled? I don't see where that is occurring. I think this is important if we silently ignore byte ranges we don't support (such as multiple byte ranges). Another option is to error instead of just logging that we don't support a byte range. |
httpcache.go
Outdated
There was a problem hiding this comment.
Instead of logging this, remove the else block from the if-statement and make its statements be the end of this function.
|
Also, in addition to the unit tests, have you tried testing this in the real world? Testing against Amazon S3 would be great - it respects range headers (although only single byte ranges, which is OK since that's all this needs to support for now). Even if it's quick manual testing. |
…ibrary Move tests to standard library
Reformat SetLogging() to SetLogger that takes a *log.Logger Check more errors in the code Set content-range header for ranged responses Add more checks to the tests (for ranges and headers) Avoid answering with a partial range to a multiple-range request, answer by refetching the request again
Add cmd/cacheproxy that contains a standalone caching proxy
|
Ping @uovobw, what's the status of this PR? |
|
@shurcooL i am responding, i am no longer working or having any contact with sourcegraph so the pr has not been updated since then. the support for the range requests was partial and i had no feedback from @sqs, i think it can be deleted as either no longer relevant or needing way too much work to be made relevant again. hope this helps, let me know |
|
Thanks for providing context @uovobw, that's helpful! |
3 participants
This change handles Range requests by replacing the response.Body object to an in-memory buffer that is created adhoc from the Range header and the size of the Body. It currently relies on the notion that the incoming range will be compliant in both form and content, a TODO has been added for future fix
The tests that are added are primed to first fetch the request to let it be stored in a new in-memory cache for each test and then re-perform the same request to verify range capabilities
I added some comments where the code might not be straightforward and go fmtd all my changes (i have not fmted the rest since it would dirty up the diff, i think it can be done at a later stage), i have used ALLCAPS for constants