Skip to content

Conversation

@Tim-Brooks
Copy link
Contributor

This is related to #34483. It introduces a namespaced setting for
compression that allows users to configure compression on a per remote
cluster basis. The transport.tcp.compress remains as a fallback
setting. If transport.tcp.compress is set to true, then all requests
and responses are compressed. If it is set to false, only requests to
clusters based on the cluster.remote.cluster_name.transport.compress
setting are compressed. However, after this change regardless of any
local settings, responses will be compressed if the request that is
received was compressed.

@Tim-Brooks Tim-Brooks added >enhancement :Distributed Coordination/Network Http and internode communication implementations v7.0.0 v6.6.0 labels Nov 8, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@Tim-Brooks
Copy link
Contributor Author

I want to make sure (in particular) that everyone agrees with this:

However, after this change regardless of any local settings, responses will be compressed if the request that is received was compressed.

This is because we do not really have a great to correlated incoming connections with settings based on cluster alias.

Second, I wanted to make sure everyone agrees that transport.tcp.compress being set to true means that everything (local and remote) is compressed. This is because it is being used as the fallback setting. With this change there is no way to only compress traffic to the local cluster. Is that something we want?

Additionally, it feels like transport.tcp.compress is a little outdated (from when we had multiple transport types). What do we feel about adding transport.compress setting that will slowly replace transport.tcp.compress?

@jasontedor @s1monw

Copy link
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

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

change LGTM. I want to clarify one thing, if we set transport.tcp.compress to true we compress all traffic in the local cluster. yet if we set it to false on a specific remote cluster we would not compress traffic to that cluster, correct?

@s1monw
Copy link
Contributor

s1monw commented Nov 8, 2018

Additionally, it feels like transport.tcp.compress is a little outdated (from when we had multiple transport types). What do we feel about adding transport.compress setting that will slowly replace transport.tcp.compress?

I agree with this, lets do this in a separate change. we should get rid of the tcp part in all the places IMO.

@jasontedor
Copy link
Member

@tbrooks8 Sharing the comment that I shared with you in another channel last night.

Additionally, it feels like transport.tcp.compress is a little outdated (from when we had multiple transport types). What do we feel about adding transport.compress setting that will slowly replace transport.tcp.compress?

I agree.

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.

LGTM.

@Tim-Brooks
Copy link
Contributor Author

yet if we set it to false on a specific remote cluster we would not compress traffic to that cluster, correct?

We will not compress requests to that cluster. If transport.tcp.compress we will compress all responses. As we do not have a way to correlate accepted server channels with settings for a specific cluster.

@Tim-Brooks Tim-Brooks merged commit 93c2c60 into elastic:master Nov 8, 2018
Tim-Brooks added a commit to Tim-Brooks/elasticsearch that referenced this pull request Nov 12, 2018
This is a follow up to elastic#35357. That commit failed to register the new
cluster.remote.cluster_name.transport.compress setting with
`ClusterSettings`. This commit fixes that.
Tim-Brooks added a commit that referenced this pull request Nov 12, 2018
This is a follow up to #35357. That commit failed to register the new
cluster.remote.cluster_name.transport.compress setting with
`ClusterSettings`. This commit fixes that.
Tim-Brooks added a commit that referenced this pull request Nov 12, 2018
This is a follow up to #35357. That commit failed to register the new
cluster.remote.cluster_name.transport.compress setting with
`ClusterSettings`. This commit fixes that.
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.

5 participants