-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(booster-http): --compression-level flag for piece & frisbii, fix GzipHandler usage #1736
Conversation
test failing because of a minor compatibility problem with [email protected], going to cut a new version soon with some other updates for my next PR so I'll roll that in here and the test will turn green |
released [email protected], updated to that in #1676 and that should address the CI problems here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will review the final PR against main more closely. Based on a quick look this looks fine 👍
I've pulled this in to #1676, it car be reviewed there as part of the mega-changeset but is still a separate commit that can be viewed separately if desired. |
This is a stacked PR on top of #1676; one of a couple I'm going to do to consolidate and address some things with the two primary HTTP handler paths.
This one does a few things:
--compression-level
flag with a default of1
. Allows users to turn it off if they're going to do this in a reverse proxy (recommended, really).gziphandler.GzipResponseWriter
in the piece HandlerFunc with agziphandler.MustNewGzipLevelHandler()
as a Handler, wrapping the HandlerFunc - this does two things: (1) lets us set a compression level, the default we're getting now is a basic deflate (1
), and (2) lets us use the gzip pool that gziphandler manages at the level of Handler.Cache-Control
.I've set the default to
1
but I'd rather set it to6
for better results, with a small amount of CPU impact. I chose1
because that's all users are getting now.