Issue 2241: support compression #2843
Conversation
dgraph/cmd/alpha/http.go
Outdated
| w.Header().Add("X-Dgraph-Write-Response", "compressing") | ||
| } | ||
|
|
||
| out.Write(b) |
There was a problem hiding this comment.
Error return value of out.Write is not checked (from errcheck)
There was a problem hiding this comment.
Reviewable status: 0 of 7 files reviewed, 9 unresolved discussions (waiting on @golangcibot, @codexnull, and @manishrjain)
dgraph/cmd/alpha/http.go, line 126 at r1 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
Error return value of
out.Writeis not checked (fromerrcheck)
return out.Write(b)
dgraph/cmd/alpha/http.go, line 67 at r2 (raw file):
} // Common functionality for these request handlers. Returns true if the request is completely handled here
100 chars.
dgraph/cmd/alpha/http.go, line 70 at r2 (raw file):
// and nothing further needs to be done. func commonHandler(w http.ResponseWriter, r *http.Request) bool { // Do these requests really need CORS headers? Doesn't seem like it, but they are probably
I think for options, we needed them. Not sure if the typical POST and PUT needs them or not.
dgraph/cmd/alpha/http.go, line 87 at r2 (raw file):
// Read request body, transparently decompressing if necessary. Return nil on error. func readReqBody(w http.ResponseWriter, r *http.Request) []byte {
Rename to readRequest
dgraph/cmd/alpha/http.go, line 105 at r2 (raw file):
} body, err := ioutil.ReadAll(in)
Think you're missing the defer r.Body.Close().
dgraph/cmd/alpha/http.go, line 115 at r2 (raw file):
// Write response body, transparently compressing if necessary. func writeRespBody(w http.ResponseWriter, r *http.Request, b []byte) {
Rename to writeResponse. Also, return error.
dgraph/cmd/alpha/http.go, line 116 at r2 (raw file):
// Write response body, transparently compressing if necessary. func writeRespBody(w http.ResponseWriter, r *http.Request, b []byte) { var out io.Writer = w
out := w
dgraph/cmd/alpha/http.go, line 120 at r2 (raw file):
if strings.Contains(r.Header.Get("Accept-Encoding"), "gzip") { w.Header().Set("Content-Encoding", "gzip") gz := gzip.NewWriter(w)
gzw
dgraph/cmd/alpha/http.go, line 152 at r2 (raw file):
} q := readReqBody(w, r)
body := readRequest(w, r)
| @@ -159,32 +213,24 @@ func queryHandler(w http.ResponseWriter, r *http.Request) { | |||
| writeEntry("data", resp.Json) | |||
| } | |||
| out.WriteRune('}') | |||
| w.Write(out.Bytes()) | |||
|
|
|||
| writeResponse(w, r, out.Bytes()) | |||
There was a problem hiding this comment.
Error return value of writeResponse is not checked (from errcheck)
| @@ -261,20 +307,12 @@ func mutationHandler(w http.ResponseWriter, r *http.Request) { | |||
| x.SetStatusWithData(w, x.Error, err.Error()) | |||
| return | |||
| } | |||
| w.Write(js) | |||
|
|
|||
| writeResponse(w, r, js) | |||
There was a problem hiding this comment.
Error return value of writeResponse is not checked (from errcheck)
| @@ -335,20 +371,12 @@ func commitHandler(w http.ResponseWriter, r *http.Request) { | |||
| x.SetStatusWithData(w, x.Error, err.Error()) | |||
| return | |||
| } | |||
| w.Write(js) | |||
|
|
|||
| writeResponse(w, r, js) | |||
There was a problem hiding this comment.
Error return value of writeResponse is not checked (from errcheck)
There was a problem hiding this comment.
Reviewable status: 0 of 7 files reviewed, 12 unresolved discussions (waiting on @golangcibot and @manishrjain)
dgraph/cmd/alpha/http.go, line 126 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
return out.Write(b)
Fixed.
dgraph/cmd/alpha/http.go, line 67 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
100 chars.
Fixed.
dgraph/cmd/alpha/http.go, line 70 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
I think for options, we needed them. Not sure if the typical POST and PUT needs them or not.
CORS headers are used to allow a resource to pull in content from a different domain. I can see that ratel (for example) needs them so that the web pages served from port 8000 can query the alphas on a different host or port, but AFAIK alpha responses don't have any external references or executable code that may try to fetch additional data.
In any case, I left them alone.
dgraph/cmd/alpha/http.go, line 87 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Rename to readRequest
Fixed.
dgraph/cmd/alpha/http.go, line 105 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Think you're missing the
defer r.Body.Close().
That doesn't seem to be necessary. From the http.Request docs:
// The Server will close the request body. The ServeHTTP
// Handler does not need to.
dgraph/cmd/alpha/http.go, line 115 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Rename to writeResponse. Also, return error.
Done.
dgraph/cmd/alpha/http.go, line 116 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
out := w
That doesn't work here because 'out' will be type http.ResponseWriter, which the gzip.Writer can't be assigned to if compressing.
dgraph/cmd/alpha/http.go, line 120 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
gzw
Done.
dgraph/cmd/alpha/http.go, line 152 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
body := readRequest(w, r)
Done.
|
Fixes #2241. |
* Enable gzipped response over HTTP. * Add test for query response compression. * Support gzipped request over HTTP. * Add test for compressed HTTP request. * Add --use_compression option with live loader. * Add simple test for gRPC compression support. * Refactor HTTP handlers to share common functionality instead of duplicating it.
Support compressed requests and responses over gRPC and HTTP.
This change is