Skip to content
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

Enable HTTP compression by default with compression level 3 #18066

Conversation

danielmitterdorfer
Copy link
Member

With this commit we compress HTTP responses provided the client
supports it (as indicated by the HTTP header 'Accept-Encoding').

We're also able to process compressed HTTP requests if needed.

The default compression level is lowered from 6 to 3 as benchmarks
have indicated that this reduces query latency with a negligible
increase in network traffic.

Closes #7309

With this commit we compress HTTP responses provided the client
supports it (as indicated by the HTTP header 'Accept-Encoding').

We're also able to process compressed HTTP requests if needed.

The default compression level is lowered from 6 to 3 as benchmarks
have indicated that this reduces query latency with a negligible
increase in network traffic.

Closes elastic#7309
pipeline.addLast("encoder", new ESHttpResponseEncoder());
if (transport.compression) {
pipeline.addLast("encoder_compress", new HttpContentCompressor(transport.compressionLevel));
}
if (SETTING_CORS_ENABLED.get(transport.settings())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to move this down? I'm not sure it makes a difference but it'd be interesting to know if it did.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is intentional. I was about to ping @abeyad whether he sees any problem with this. I did this because the CORS tests (i.e. CorsRegexIT) broke with HTTP compression enabled. The reason was that HttpContentCompressor expected an Accepts-Encoding header that was not there anymore and then failed.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielmitterdorfer the change in itself looks fine, as all you're doing is moving the CORS handling in the pipeline to after HTTP compression, but its still before the request handling which is the most important thing. But I'm curious as to why the Accepts-Encoding header was not available and becomes available if you move the CORS handling down one notch?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abeyad Sorry that I did not state it clearly enough. The header itself would still be available but it is read by HttpContentEncoder#messageReceived() was not called if the CORS handler came before in the pipeline. When the response is about to be sent, Netty calls HttpContentEncoder#messageReceived() which relies on a field being set by messageReceived(). That's what caused the problem.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielmitterdorfer Yes I see what you mean now. I think the change makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks for double-checking.

@jimczi
Copy link
Contributor

jimczi commented May 2, 2016

@danielmitterdorfer maybe we should add a small note on the breaking changes about the fact that compressed request will not check the http.compression setting anymore. I think it's important because this means that any client could send a compressed request without checking if the compression is disabled or not on the cluster.

@jimczi
Copy link
Contributor

jimczi commented May 2, 2016

I've left a tiny comment but the change looks great.
LGTM

public static final Setting<Integer> SETTING_HTTP_COMPRESSION_LEVEL =
Setting.intSetting("http.compression_level", 6, Property.NodeScope);
Setting.intSetting("http.compression_level", 3, Property.NodeScope);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe leave a comment about why 3. It might seem weird otherwise that our default is different from the default of zlib.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. I'll add a comment.

@danielmitterdorfer
Copy link
Member Author

@jimferenczi I have documented the change now in the breaking changes docs. As I already got a LGTM, I'll merge the change soon. Thanks a lot for your review.

danielmitterdorfer added a commit to danielmitterdorfer/elasticsearch that referenced this pull request May 3, 2016
With this commit we fix an issue that prevented to turn on
HTTP compression when using CORS. The problem boiled down
to a problematic order of the respective handlers in
Netty's pipeline.

Note that the same problem is already fixed in ES 5.0 by

Relates elastic#18066
Fixes elastic#18089
danielmitterdorfer added a commit that referenced this pull request May 3, 2016
With this commit we fix an issue that prevented to turn on
HTTP compression when using CORS. The problem boiled down
to a problematic order of the respective handlers in
Netty's pipeline.

Note that the same problem is already fixed in ES 5.0 by

Relates #18066
Fixes #18089
@danielmitterdorfer
Copy link
Member Author

Closed by 0a6f40c.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants