darion: Use http.ServeContent for serving log data #102
Conversation
|
Hrm. Gotta figure out how to fix this. |
cHYzZQo
force-pushed
the
range-requests
branch
2 times, most recently
from
98b9db4
to
f800a25
Compare
Pull Request Test Coverage Report for Build 799
💛 - Coveralls |
Codecov Report
@@ Coverage Diff @@
## master #102 +/- ##
===========================================
- Coverage 34.97% 24.29% -10.68%
===========================================
Files 65 64 -1
Lines 7302 7157 -145
===========================================
- Hits 2554 1739 -815
- Misses 4439 5219 +780
+ Partials 309 199 -110
|
|
FYI, this is for TITUS-1922 (which I realize doesn't help anyone external to Netflix). |
|
@cHYzZQo, looks like the CI tests are now passing. Ready for review? |
|
@cHYzZQo Just curious -- do you need this both on stdio and normal log files, or just on normal log files? |
|
@sargun yes ready for review. Both stdio and normal log files would be good. |
|
Okay, let me mull this over and I'll look at it EOD. |
darion/api/handlers_test.go
Outdated
| } | ||
|
|
||
| resp, e := client.Do(req) | ||
| if e != nil { |
There was a problem hiding this comment.
| @@ -168,7 +215,7 @@ func verifyHelper(resp *http.Response, t *testing.T) string { | |||
| t.Fatal("Could not read response body: ", err) | |||
| } | |||
| dataStr := string(data) | |||
| if resp.StatusCode != 200 { | |||
There was a problem hiding this comment.
When will we receive not 200? When it's range request not satisfiable?
There was a problem hiding this comment.
If that's the case, we should probably explicitly check for that.
There was a problem hiding this comment.
If a range is requested http specs say the server should respond with a 206 "Partial Content".
In general though, any code between 200 and 399 is considered a success. And success checks should allow for anything in that range. For example 304s would be pretty common. But I can change the test to only allow 200 or 206.
There was a problem hiding this comment.
BTW the code for unsatisfiable range requests is 416.
There was a problem hiding this comment.
Do we want add a specific test checking for the 416 case, and for the 206 case?
There was a problem hiding this comment.
Sure, added. Though at that point we are testing the http lib not our code.
cHYzZQo
force-pushed
the
range-requests
branch
2 times, most recently
from
e644a86
to
c3d2cba
Compare
This allows us to support range serving ranges
cHYzZQo
dismissed
sargun’s stale review
Unfortunatly i think we need to support virtual files for stdout and err. Made the other suggested changes
This allows us to support range serving ranges.
I'm not sure I fully understood the way the virtual file mapping is to supposed to work, so while I believe the behavior will be the same as the old code, that part might need some extra scrutiny.