Skip to content

Always compress based on the settings#36522

Merged
Tim-Brooks merged 5 commits intoelastic:masterfrom
Tim-Brooks:fix_compression_regression
Dec 12, 2018
Merged

Always compress based on the settings#36522
Tim-Brooks merged 5 commits intoelastic:masterfrom
Tim-Brooks:fix_compression_regression

Conversation

@Tim-Brooks
Copy link
Copy Markdown
Contributor

@Tim-Brooks Tim-Brooks commented 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 added blocker v7.0.0 v6.6.0 :Distributed/Network Http and internode communication implementations labels Dec 12, 2018
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Dec 12, 2018

Pinging @elastic/es-distributed

@Tim-Brooks
Copy link
Copy Markdown
Contributor Author

Tim-Brooks commented Dec 12, 2018 •
edited
Loading

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.

jasontedor
jasontedor approved these changes Dec 12, 2018
View reviewed changes
Copy link
Copy Markdown
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.

ywelsch
ywelsch approved these changes Dec 12, 2018
View reviewed changes
Copy link
Copy Markdown
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
@Tim-Brooks
Always compress based on the settings (elastic#36522)
35ea94f 
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
@Tim-Brooks
Always compress based on the settings (#36522) (#36566)
81720d4 
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 mentioned this pull request Dec 13, 2018
Closed
@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

Reviewers

@jasontedor jasontedor jasontedor approved these changes

@original-brownbear original-brownbear Awaiting requested review from original-brownbear

+2 more reviewers

@s1monw s1monw s1monw approved these changes

@ywelsch ywelsch ywelsch approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

blocker >bug :Distributed/Network Http and internode communication implementations v6.6.0 v7.0.0-beta1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Rally PMC performance regression due to compression

6 participants

@Tim-Brooks @elasticmachine @s1monw @ywelsch @jasontedor @colings86