Disable TCP compression when shipping Lucene segments during peer recovery

[ywelsch]

Shipping lucene segments during peer recovery is configured to set the transport compress options to false:

elasticsearch/server/src/main/java/org/elasticsearch/indices/recovery/RemoteRecoveryTargetHandler.java

Lines 70 to 71 in 7f473b6

	.withCompress(false) // lucene files are already compressed and therefore compressing this won't really help much so
	// we are saving the cpu for other things

Unfortunately this is overridden when transport.tcp.compress is enabled, discarding the request-level option:

elasticsearch/server/src/main/java/org/elasticsearch/transport/TcpTransport.java

Lines 873 to 875 in 7f473b6

	if (compress) {
	options = TransportRequestOptions.builder(options).withCompress(true).build();
	}

This means that when transport.tcp.compress is enabled, the (already compressed) Lucene files are unnecessarily compressed a second time, slowing peer recovery.

My preference would be that transport options that were explicitly set (withCompress(true) or withCompress(false)) take precedence over transport.tcp.compress.

Relates to https://discuss.elastic.co/t/elasticsearch-6-3-0-shard-recovery-is-slow/140940/

[elasticmachine]

Pinging @elastic/es-core-infra

[elasticmachine]

Pinging @elastic/es-distributed

[jpountz]

+1 to make transport options have precedence over the setting.

It's not completely obvious to me that Lucene files don't need further compression: Lucene files are indeed compressed, but in a way that still provides quick (often random) access, which prevents the effectiveness of compression techniques that may be used. Re-compressiong on top of Lucene might bring further space savings. It might still be the wrong trade-off, especially on a local network, but I wanted to clarify this point.

[jasontedor]

I agree that transport options set explicitly on the request should override the transport.tcp.compress setting. Like @jpountz, I am unsure that disabling compression on file-based recovery transfers is the right trade-off. I would like to see some experiments here. So we might end up with two changes:

• request options override transport.tcp.compress
• stop disabling compression on recovery

[jasontedor]

cc @original-brownbear @tbrooks8

[original-brownbear]

@ywelsch @jasontedor I experimented with compression performance today and there's hard data provided by @DaveCTurner provided in a linked issue (more details there) and both my experiments and the provided data suggest that compression severely limits the network throughput to around 20-30MB/s (~160-240Mbit/s) with only trivial amounts of network traffic saved (this was the result of some miscommunication, see next comment in a few min) => it seems reasonable to just fix the request options to override the transport.tcp.compress and keep disabling compression on recovery.
Also, there is the anecdotal evidence in https://discuss.elastic.co/t/elasticsearch-6-3-0-shard-recovery-is-slow/140940/14 suggesting that compression is an issue here in practice.

[original-brownbear]

@ywelsch thanks for pointing out the issue with the data I worked from :)
There's definitely a traffic saving here from compression. Measured this with the example log dataset we provide in the docs and its about a 50% reduction in traffic from compression for the whole recovery process end-to-end.

This makes the situation a little closer I guess :)
Still given the data we gathered on the runtime of recoveries and the reported issues, I think overall turning off compression wins (easier on the CPU and much faster on a 1Gbit link). It seems enabling the compression really only helps in situations available bandwidth for recovery is limited.

[ywelsch]

We discussed this during FixitFriday today. While re-compression on top of Lucene can bring further space savings (needs more benchmarks, but numbers between 10% to 50% reduction in size were mentioned), it comes at the cost of additional CPU usage, which 1) can have a negative impact on indexing / search performance, as recovery is supposed to be a background task and 2) due to the way we're sequentially sending files during recovery, can limit the throughput of single shard recoveries. While we're less enthusiastic about adding a new configuration option just for recoveries, we want to explore always disabling compression for phase 1 of recoveries, even if transport.tcp.compress is enabled. This also looks to be the thing that the code currently intends on doing, even though it's not actually working that way. The first step will therefore be to figure out whether this behavior was changed in a previous ES version.

[original-brownbear]

@ywelsch I looked into 2.4 and it seems the same issues this PR points out is present there as well:

The global setting overrides the per request option exactly like it still does, see https://github.com/elastic/elasticsearch/blob/2.4/core/src/main/java/org/elasticsearch/transport/netty/NettyTransport.java#L832.

So while we even did have the option (removed in #15235) to set compression off for the recovery it had no effect in practice with compression enabled as far as I can tell.

=> seems like this issue isn't new at all.

---

That said in the real world, I'd still vote for turning off compression for shipping the segments even when compression is globally enabled:

• Keeping the compression on here at best saves 50% traffic or speeds up recovery by 50% assuming the performance is gated by bandwidth.
• While turning it off gives you a ~10x speedup if bandwidth is not an issue.
  • And there have been a number of complaints now about the performance of recovery with compression enabled.

=> IMO it's consistent to turn off compression for data that gets little benefit out of it in this special case.