Skip to content

Always compress based on the settings #36522

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

Merged
merged 5 commits into from
Dec 12, 2018

Conversation

Tim-Brooks
Copy link
Contributor

Currently TransportRequestOptions allows specific requests to request
compression. This commit removes this and always compresses based on the
settings. Additionally, it removes TransportResponseOptions as they
are unused.

This closes #36399.

@Tim-Brooks Tim-Brooks added blocker v7.0.0 v6.6.0 :Distributed Coordination/Network Http and internode communication implementations labels Dec 12, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@Tim-Brooks
Copy link
Contributor Author

Tim-Brooks commented Dec 12, 2018

Background

This approach is based on the discussion in #36399. I want to point out that it is kind of different than the approaches discussed in #33844. In #33844 there was agreement that the TransportRequestOptions should be respected. However, this removes the compression option.

Just so the rules are understood here:

Behavior

Requests

transport.tcp.compress - is the default setting. It defines whether the local cluster requests are compressed. It is also the fallback setting for remote clusters. This setting will probably change to transport.compress in 7.0.

cluster.remote.name.transport.compress - is the setting for remote clusters. If this is set for a remote cluster, it is the setting that is used.

Responses

We compress the response IF the request was compressed. This is a change from prior (6.5) behavior which would only compress responses if transport.tcp.compress was set to true. This is because we do not know what setting to apply, as we do not know if an inbound connection is remote or local.

Also, we will ALWAYS compress responses if transport.tcp.compress is set to true. Do we want this behavior? This is to be compatible with the prior behavior where if transport.tcp.compress was set to true, responses would be compressed. But maybe we should move to a world where we always do what the request sender is doing?

Blocker

This should be considered a blocker for 6.6 as the current behavior will lead to many more requests being compressed than in 6.5. And that will lead to unexpected performance degradation.

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

Looks good, left one comment twice. No need for another round.

Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

Thanks @tbrooks8

@Tim-Brooks Tim-Brooks merged commit 7f612d5 into elastic:master Dec 12, 2018
Tim-Brooks added a commit to Tim-Brooks/elasticsearch that referenced this pull request Dec 12, 2018
Currently TransportRequestOptions allows specific requests to request
compression. This commit removes this and always compresses based on the
settings. Additionally, it removes TransportResponseOptions as they
are unused.

This closes elastic#36399.
Tim-Brooks added a commit that referenced this pull request Dec 12, 2018
Currently TransportRequestOptions allows specific requests to request
compression. This commit removes this and always compresses based on the
settings. Additionally, it removes TransportResponseOptions as they
are unused.

This closes #36399.
@Tim-Brooks Tim-Brooks deleted the fix_compression_regression branch December 18, 2019 14:46
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.

Rally PMC performance regression due to compression
6 participants