Only compress responses if request was compressed by Tim-Brooks · Pull Request #36867 · elastic/elasticsearch

Tim-Brooks · 2018-12-19T23:01:43Z

This is a follow-up to some discussions around #36399. Currently we have
relatively confusing compression behavior where compression can be
configured for requests based on transport.compress or a specific
setting for a remote cluster. However, we can only compress responses
based on transport.compress as we do not know where a request is
coming from (currently).

This commit modifies the behavior to NEVER compress responses based on
settings. Instead, a response will only be compressed if the request was
compressed. This commit also updates the documentation to more clearly
described transport level compression.

This is a follow-up to some discussions around elastic#36399. Currently we have relatively confusing compression behavior where compression can be configured for requests based on `transport.compress` or a specific setting for a remote cluster. However, we can only compress responses based on `transport.compress` as we do not know where a request is coming from (currently). This commit modifies the behavior to NEVER compress responses based on settings. Instead, a response will only be compressed if the request was compressed. This commit also updates the documentation to more clearly described transport level compression.

elasticmachine · 2018-12-19T23:01:44Z

Pinging @elastic/es-distributed

Tim-Brooks · 2018-12-19T23:05:41Z

This PR is based on a conversation with I had with @jasontedor where I suggested it might be nicer to have simpler rules for compressing responses. Instead of having this odd edge-case where only some of the compression settings apply to responses, none of the compression settings will apply to responses 🤣.

I wanted to open this PR and see if others agreed. If we all agree and generally there is agreement that the description about compression I added to the documentation is correct, I'll add @lcawl as a specific reviewer to iron-out the language. But @lcawl - you can hold off reviewing until we agree that this is a change we want to make.

…uests

ywelsch · 2018-12-20T11:16:39Z

The change makes sense to me. Can you also add tests that check that we're getting the desired behavior with this change?

jasontedor

LGTM.

jasontedor · 2018-12-20T11:20:20Z

docs/reference/modules/transport.asciidoc

+`cluster.remote.${cluster_alias}.transport.compress` setting.
+
+When compression is enabled using either `transport.compress` or for a specific
+remote cluster, only the outbound requests are compressed. Elasticsearch only


Elasticsearch only -> Elasticsearch

original-brownbear

LGTM2

Tim-Brooks · 2018-12-20T17:00:24Z

@lcawl - I think that we're at a point where it is probably worth you reviewing the documentation.

lcawl · 2018-12-20T23:40:11Z

docs/reference/modules/transport.asciidoc

+noticeable CPU cost and local clusters tend to be setup with fast network
+connections between nodes. If compression is necessary for remote cluster
+connections only, this can be configured on a per-cluster basis using the
+`cluster.remote.${cluster_alias}.transport.compress` setting.


Suggested change

`cluster.remote.${cluster_alias}.transport.compress` setting.

<<remote-cluster-settings,`cluster.remote.${cluster_alias}.transport.compress` setting>>.

lcawl · 2018-12-20T23:42:33Z

docs/reference/modules/transport.asciidoc

+[float]
+==== Transport Compression
+
+By default Elasticsearch network-level compression is disabled. This default


Suggested change

By default Elasticsearch network-level compression is disabled. This default

By default, the `transport.compress` setting is `false` and network-level compression is disabled between nodes in the cluster. This default

lcawl · 2018-12-20T23:44:24Z

docs/reference/modules/transport.asciidoc

+
+By default Elasticsearch network-level compression is disabled. This default
+normally makes sense for local cluster communication as compression has a
+noticeable CPU cost and local clusters tend to be setup with fast network


Suggested change

noticeable CPU cost and local clusters tend to be setup with fast network

noticeable CPU cost and local clusters tend to be set up with fast network

lcawl · 2018-12-20T23:53:34Z

docs/reference/modules/transport.asciidoc

+By default Elasticsearch network-level compression is disabled. This default
+normally makes sense for local cluster communication as compression has a
+noticeable CPU cost and local clusters tend to be setup with fast network
+connections between nodes. If compression is necessary for remote cluster


Suggested change

connections between nodes. If compression is necessary for remote cluster

connections between nodes. If you want to compress remote cluster

lcawl · 2018-12-20T23:56:04Z

docs/reference/modules/transport.asciidoc

+normally makes sense for local cluster communication as compression has a
+noticeable CPU cost and local clusters tend to be setup with fast network
+connections between nodes. If compression is necessary for remote cluster
+connections only, this can be configured on a per-cluster basis using the


The "only" here seems to imply that setting transport.compress affects both internode and remote cluster communications. i.e. you set this cluster-level setting if you only want to compress remote cluster connections. If that's not right, we should remove the "only".

lcawl · 2018-12-21T00:02:38Z

docs/reference/modules/transport.asciidoc

+`cluster.remote.${cluster_alias}.transport.compress` setting.
+
+When compression is enabled using either `transport.compress` or for a specific
+remote cluster, only the outbound requests are compressed. Elasticsearch compresses


Suggested change

remote cluster, only the outbound requests are compressed. Elasticsearch compresses

remote cluster, only the outbound requests are compressed.

lcawl · 2018-12-21T00:04:28Z

docs/reference/modules/transport.asciidoc

+
+When compression is enabled using either `transport.compress` or for a specific
+remote cluster, only the outbound requests are compressed. Elasticsearch compresses
+responses if and only-if the inbound request received was compressed.


Suggested change

responses if and only-if the inbound request received was compressed.

If compression is enabled and an inbound request is compressed, {es} also compresses the response.

Not sure if it's true that the compression of responses requires compression to be enabled, but it seemed like a reasonable assumption. If that's not true, I'd suggest something like this:

Suggested change

responses if and only-if the inbound request received was compressed.

If an inbound request is compressed, {es} compresses the response--even when compression is not enabled.

…uests

Tim-Brooks · 2018-12-21T00:48:47Z

@lcawl - I made some changes. I went ahead and split it into request and response sections to be completely clear.

lcawl

LGTM

Tim-Brooks · 2018-12-21T17:13:50Z

Thanks @lcawl

This is a follow-up to some discussions around elastic#36399. Currently we have relatively confusing compression behavior where compression can be configured for requests based on transport.compress or a specific setting for a remote cluster. However, we can only compress responses based on transport.compress as we do not know where a request is coming from (currently). This commit modifies the behavior to NEVER compress responses based on settings. Instead, a response will only be compressed if the request was compressed. This commit also updates the documentation to more clearly described transport level compression.

* elastic/master: (539 commits) SQL: documentation improvements and updates (elastic#36918) [DOCS] Merges list of discovery and cluster formation settings (elastic#36909) Only compress responses if request was compressed (elastic#36867) Remove duplicate paragraph (elastic#36942) Fix URI to cluster stats endpoint on specific nodes (elastic#36784) Fix typo in unitTest task (elastic#36930) RecoveryMonitor#lastSeenAccessTime should be volatile (elastic#36781) [CCR] Add `ccr.auto_follow_coordinator.wait_for_timeout` setting (elastic#36714) Scripting: Remove deprecated params.ctx (elastic#36848) Refactor the REST actions to clarify what endpoints are deprecated. (elastic#36869) Add JDK 12 to CI rotation (elastic#36915) Improve error message for 6.x style realm settings (elastic#36876) Send clear session as routable remote request (elastic#36805) [DOCS] Remove redundant ILM attributes (elastic#36808) SQL: Fix bug regarding histograms usage in scripting (elastic#36866) Update index mappings when ccr restore complete (elastic#36879) Docs: Bump version to alpha2 after release Enable IPv6 URIs in reindex from remote (elastic#36874) Watcher: Remove unused local variable in doExecute (elastic#36655) [DOCS] Synchs titles of X-Pack APIs ...

* master: (31 commits) Move ingest-geoip default databases out of config (elastic#36949) [ILM][DOCS] add extra scenario to policy update docs (elastic#36871) [Painless] Add String Casting Tests (elastic#36945) SQL: documentation improvements and updates (elastic#36918) [DOCS] Merges list of discovery and cluster formation settings (elastic#36909) Only compress responses if request was compressed (elastic#36867) Remove duplicate paragraph (elastic#36942) Fix URI to cluster stats endpoint on specific nodes (elastic#36784) Fix typo in unitTest task (elastic#36930) RecoveryMonitor#lastSeenAccessTime should be volatile (elastic#36781) [CCR] Add `ccr.auto_follow_coordinator.wait_for_timeout` setting (elastic#36714) Scripting: Remove deprecated params.ctx (elastic#36848) Refactor the REST actions to clarify what endpoints are deprecated. (elastic#36869) Add JDK 12 to CI rotation (elastic#36915) Improve error message for 6.x style realm settings (elastic#36876) Send clear session as routable remote request (elastic#36805) [DOCS] Remove redundant ILM attributes (elastic#36808) SQL: Fix bug regarding histograms usage in scripting (elastic#36866) Update index mappings when ccr restore complete (elastic#36879) Docs: Bump version to alpha2 after release ...

This is a follow-up to some discussions around #36399. Currently we have relatively confusing compression behavior where compression can be configured for requests based on transport.compress or a specific setting for a remote cluster. However, we can only compress responses based on transport.compress as we do not know where a request is coming from (currently). This commit modifies the behavior to NEVER compress responses based on settings. Instead, a response will only be compressed if the request was compressed. This commit also updates the documentation to more clearly described transport level compression.

Tim-Brooks added >non-issue :Distributed/Network Http and internode communication implementations v7.0.0 v6.7.0 labels Dec 19, 2018

Tim-Brooks requested review from jasontedor, original-brownbear, s1monw and ywelsch December 19, 2018 23:01

Tim-Brooks added 2 commits December 19, 2018 16:12

cleanup

02e4243

Merge remote-tracking branch 'upstream/master' into only_compress_req…

554fb54

…uests

jasontedor approved these changes Dec 20, 2018

View reviewed changes

original-brownbear approved these changes Dec 20, 2018

View reviewed changes

Add test

87dfb3b

Tim-Brooks requested a review from lcawl December 20, 2018 16:59

s1monw approved these changes Dec 20, 2018

View reviewed changes

Add suppress

9f10684

lcawl reviewed Dec 20, 2018

View reviewed changes

lcawl reviewed Dec 21, 2018

View reviewed changes

Tim-Brooks added 2 commits December 20, 2018 17:18

Merge remote-tracking branch 'upstream/master' into only_compress_req…

ac4838a

…uests

Changes

fd291d4

lcawl approved these changes Dec 21, 2018

View reviewed changes

Tim-Brooks merged commit c8a8391 into elastic:master Dec 21, 2018

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Tim-Brooks deleted the only_compress_requests branch December 18, 2019 14:47

	`cluster.remote.${cluster_alias}.transport.compress` setting.
	<<remote-cluster-settings,`cluster.remote.${cluster_alias}.transport.compress` setting>>.

	By default Elasticsearch network-level compression is disabled. This default
	By default, the `transport.compress` setting is `false` and network-level compression is disabled between nodes in the cluster. This default

	noticeable CPU cost and local clusters tend to be setup with fast network
	noticeable CPU cost and local clusters tend to be set up with fast network

	connections between nodes. If compression is necessary for remote cluster
	connections between nodes. If you want to compress remote cluster

	remote cluster, only the outbound requests are compressed. Elasticsearch compresses
	remote cluster, only the outbound requests are compressed.

	responses if and only-if the inbound request received was compressed.
	If compression is enabled and an inbound request is compressed, {es} also compresses the response.

	responses if and only-if the inbound request received was compressed.
	If an inbound request is compressed, {es} compresses the response--even when compression is not enabled.

Conversation

Tim-Brooks commented Dec 19, 2018

Uh oh!

elasticmachine commented Dec 19, 2018

Uh oh!

Tim-Brooks commented Dec 19, 2018

Uh oh!

ywelsch commented Dec 20, 2018

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

jasontedor Dec 20, 2018

Choose a reason for hiding this comment

Uh oh!

original-brownbear left a comment

Choose a reason for hiding this comment

Uh oh!

Tim-Brooks commented Dec 20, 2018

Uh oh!

lcawl Dec 20, 2018

Choose a reason for hiding this comment

Uh oh!

lcawl Dec 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lcawl Dec 20, 2018

Choose a reason for hiding this comment

Uh oh!

lcawl Dec 20, 2018

Choose a reason for hiding this comment

Uh oh!

lcawl Dec 20, 2018

Choose a reason for hiding this comment

Uh oh!

lcawl Dec 21, 2018

Choose a reason for hiding this comment

Uh oh!

lcawl Dec 21, 2018

Choose a reason for hiding this comment

Uh oh!

lcawl Dec 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tim-Brooks commented Dec 21, 2018

Uh oh!

lcawl left a comment

Choose a reason for hiding this comment

Uh oh!

Tim-Brooks commented Dec 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

lcawl Dec 20, 2018 •

edited

Loading

lcawl Dec 21, 2018 •

edited

Loading