Don't pretty-print by default. Saves an allocation (or more) and a copy. #9361

brendandburns · 2015-06-06T06:03:55Z

@smarterclayton @wojtek-t @fgrzadkowski @lavalamp

k8s-bot · 2015-06-06T06:06:32Z

EXPERIMENTAL JENKINS PR BUILDER: e2e build succeeded.

smarterclayton · 2015-06-06T15:15:44Z

Post 1.0 we should switch to letting go restful serialize our response which reduces the cost of this.

smarterclayton · 2015-06-06T15:16:51Z

pkg/apiserver/proxy.go

@@ -56,6 +56,7 @@ func (r *ProxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 	var apiResource string
 	var httpCode int
 	reqStart := time.Now()
+	pretty := isPrettyPrint(req)


A little concerned about proxy looking at query parameters. I would say proxy shouldn't do this (otherwise if he backend also supports pp it's going to get mangled).

I guess since its errors it's less impactful...

Or we could just make indentation true by default for proxy errors.

Yeah, I think that makes the most sense. Until we have larger errors (I have a todo to make an update conflict return the conflicting resource) we can get away with always indenting errors for a lot of code.

On Jun 6, 2015, at 12:14 PM, Brian Grant notifications@github.com wrote:

In pkg/apiserver/proxy.go:

@@ -56,6 +56,7 @@ func (r *ProxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
var apiResource string
var httpCode int
reqStart := time.Now()

pretty := isPrettyPrint(req)
Or we could just make indentation true by default for proxy errors.

—
Reply to this email directly or view it on GitHub.

switched to default true.

bgrant0607 · 2015-06-06T16:08:28Z

Ref. most recent discussion: #4521 (comment)

Original discussion: #2243

bgrant0607 · 2015-06-06T16:10:46Z

pkg/apiserver/apiserver.go

+}
+
+func isPrettyPrint(req *http.Request) bool {
+	pp := req.URL.Query().Get("pp")


We don't abbreviate in the API. I suggest "indent".

I also suggest that we pretty print in the case that User-Agent is "curl/.*"

Or "pretty" rather than "indent".

done. Added user agents for modern browsers and cli clients.

Did you push the new code yet?

just pushed... Sorry, I was 'done-ing' comments as I came to them, and I needed to add the swagger docs you asked for...

bgrant0607 · 2015-06-06T16:29:12Z

Users are going to want to know that this query parameter exists. We should document it in the swagger. I put some pointers here:
#3714 (comment)

bgrant0607 · 2015-06-06T16:59:28Z

@smarterclayton Why do you think go-restful would be more efficient?
https://github.com/emicklei/go-restful/blob/5aeddab2ca153e662be4ff9efb82f6c6788466d0/response.go#L159

Or are you just suggesting that we marshal and indent in the same step, as go-restful does? We could pass the pretty flag to Encode, or factor out marshaling, which we'd also have to do if we were going to call go-restful's Write function.

smarterclayton · 2015-06-06T17:05:16Z

On Jun 6, 2015, at 12:59 PM, Brian Grant notifications@github.com wrote:

@smarterclayton Why do you think go-restful would be more efficient?
https://github.com/emicklei/go-restful/blob/5aeddab2ca153e662be4ff9efb82f6c6788466d0/response.go#L159

I just meant we should use gorestfuls library methods and improve them as needed - instead of us writing directly to the response use resp.WriteEntity etc.
Or are you just suggesting that we marshal and indent in the same step, as go-restful does? We could pass the pretty flag to Encode, or factor out marshaling, which we'd also have to do if we were going to call go-restful's Write function.

In the long run we want to factor marshaling out anyway (to support protobuf or similar). Not a short term thing though, just bringing up we we don't necessarily want to do all our own response writing forever.
—
Reply to this email directly or view it on GitHub.

brendandburns · 2015-06-08T23:30:36Z

Comments addressed and code pushed, ptal.

--brendan

brendandburns · 2015-06-08T23:31:44Z

ugh, something in the pre-check borked... hang on.

bgrant0607 · 2015-06-08T23:32:55Z

Yeah, deep copy vaporized.

brendandburns · 2015-06-08T23:35:08Z

ok, should be fixed now. Sorry about that.

k8s-bot · 2015-06-08T23:36:42Z

EXPERIMENTAL JENKINS PR BUILDER: e2e build succeeded.

derekwaynecarr · 2015-06-08T23:39:35Z

@smarterclayton - should I add this to our list of items to cherry pick? Seems useful

k8s-bot · 2015-06-08T23:46:48Z

EXPERIMENTAL JENKINS PR BUILDER: e2e build succeeded.

bgrant0607 · 2015-06-08T23:57:50Z

pkg/apiserver/api_installer.go

@@ -496,6 +502,7 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
 			route := ws.GET(action.Path).To(ListResource(lister, watcher, reqScope, true, a.minRequestTimeout)).
 				Filter(m).
 				Doc("watch changes to an object of kind "+kind).
+				Param(ws.QueryParameter("pretty", "If 'true', then the output is pretty printed.")).


We should add Param to WATCHLIST, also, for consistency.

done (it's marked deprecated so I skipped it...)

bgrant0607 · 2015-06-09T00:08:25Z

Thanks. Just a couple minor details to address.

Also, PR #9388 was merged, so you'll need to update the generated swagger in order to merge.

bgrant0607 · 2015-06-09T00:18:29Z

And, if #9389 is merged, you'll need to rebase and regenerate the swagger again.

cc @nikhiljindal @krousey

brendandburns · 2015-06-09T04:33:31Z

Rebased, and regenerated swagger. ptal.

Thanks!
--brendan

k8s-bot · 2015-06-09T04:36:36Z

EXPERIMENTAL JENKINS PR BUILDER: e2e build succeeded.

bgrant0607 · 2015-06-09T13:31:07Z

LGTM, thanks.

Don't pretty-print by default. Saves an allocation (or more) and a copy.

googlebot added the cla: yes label Jun 6, 2015

smarterclayton reviewed Jun 6, 2015
View reviewed changes

bgrant0607 reviewed Jun 6, 2015
View reviewed changes

bgrant0607 self-assigned this Jun 6, 2015

bgrant0607 assigned smarterclayton and unassigned bgrant0607 Jun 8, 2015

brendandburns force-pushed the perf3 branch from 75161d5 to c17cbfa Compare June 8, 2015 23:31

brendandburns force-pushed the perf3 branch from c17cbfa to 1efca37 Compare June 8, 2015 23:32

brendandburns force-pushed the perf3 branch from 1efca37 to 50e23a3 Compare June 8, 2015 23:34

bgrant0607 reviewed Jun 8, 2015
View reviewed changes

Don't pretty-print by default. Saves an allocation (or more) and a copy.

b68ddba

brendandburns force-pushed the perf3 branch from 50e23a3 to b68ddba Compare June 9, 2015 04:32

bgrant0607 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2015

bgrant0607 assigned bgrant0607 and unassigned smarterclayton Jun 9, 2015

krousey added a commit that referenced this pull request Jun 9, 2015

Merge pull request #9361 from brendandburns/perf3

cf19420

Don't pretty-print by default. Saves an allocation (or more) and a copy.

krousey merged commit cf19420 into kubernetes:master Jun 9, 2015

This was referenced Jun 9, 2015

Finalize Kube items openshift/origin#2233

Closed

UPSTREAM Don't pretty print by default openshift/origin#3095

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't pretty-print by default. Saves an allocation (or more) and a copy. #9361

Don't pretty-print by default. Saves an allocation (or more) and a copy. #9361

brendandburns commented Jun 6, 2015

k8s-bot commented Jun 6, 2015

smarterclayton commented Jun 6, 2015

smarterclayton Jun 6, 2015

smarterclayton Jun 6, 2015

bgrant0607 Jun 6, 2015

smarterclayton Jun 6, 2015

brendandburns Jun 8, 2015

bgrant0607 commented Jun 6, 2015

bgrant0607 Jun 6, 2015

bgrant0607 Jun 6, 2015

bgrant0607 Jun 6, 2015

brendandburns Jun 8, 2015

bgrant0607 Jun 8, 2015

brendandburns Jun 8, 2015

bgrant0607 commented Jun 6, 2015

bgrant0607 commented Jun 6, 2015

smarterclayton commented Jun 6, 2015

brendandburns commented Jun 8, 2015

brendandburns commented Jun 8, 2015

bgrant0607 commented Jun 8, 2015

brendandburns commented Jun 8, 2015

k8s-bot commented Jun 8, 2015

derekwaynecarr commented Jun 8, 2015

k8s-bot commented Jun 8, 2015

bgrant0607 Jun 8, 2015

brendandburns Jun 9, 2015

bgrant0607 commented Jun 9, 2015

bgrant0607 commented Jun 9, 2015

brendandburns commented Jun 9, 2015

k8s-bot commented Jun 9, 2015

bgrant0607 commented Jun 9, 2015

Don't pretty-print by default. Saves an allocation (or more) and a copy. #9361

Don't pretty-print by default. Saves an allocation (or more) and a copy. #9361

Conversation

brendandburns commented Jun 6, 2015

k8s-bot commented Jun 6, 2015

smarterclayton commented Jun 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgrant0607 commented Jun 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgrant0607 commented Jun 6, 2015

bgrant0607 commented Jun 6, 2015

smarterclayton commented Jun 6, 2015

brendandburns commented Jun 8, 2015

brendandburns commented Jun 8, 2015

bgrant0607 commented Jun 8, 2015

brendandburns commented Jun 8, 2015

k8s-bot commented Jun 8, 2015

derekwaynecarr commented Jun 8, 2015

k8s-bot commented Jun 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgrant0607 commented Jun 9, 2015

bgrant0607 commented Jun 9, 2015

brendandburns commented Jun 9, 2015

k8s-bot commented Jun 9, 2015

bgrant0607 commented Jun 9, 2015