Add compression to replicator - #6013
Conversation
|
Can you please use our PR template? |
There was a problem hiding this comment.
Nice work @lacklacklack!
A few suggestions / ideas as bullet points:
-
We're trying to do a bit of both: request and response sides; let's keep it simpler at first and focus on the request side (_bulk_docs and _revs_diff). Since we already have server side handling for gzip, let's handle gzip only for the request side. Then we can cleanly test the feature in CI without extra proxies or having to add server side compression sending here too.
-
Let's skip
deflateand use gzip only. But add the possibility to addzstdin the future (not this PR though) so keep the configurable compression algorithm. The reason to skip deflate is simplicity (our server handles gzip only), and deflate is a bit of a mess according to https://en.wikipedia.org/wiki/HTTP_compression
Another problem found while deploying HTTP compression on large scale is due to the deflate encoding definition: while HTTP 1.1 defines the deflate encoding as data compressed with deflate (RFC 1951) inside a zlib formatted stream (RFC 1950), Microsoft server and client products historically implemented it as a "raw" deflated stream making its deployment unreliable. For this reason, some software, including the Apache HTTP Server, only implements gzip encoding.
-
As it stands, the most important thing to compress (_bulk_docs body) won't actually be compressed. The body there isn't an iolist or binary but
{BodyFun, [prefix | Docs]}}. So that makes me think maybe a better place for this is not in httpc but in api_wrap -
Do not set
AcceptEncodings = config:get("replicator", "accept_encodings", "gzip, deflate, zstd")unless we can always handle these responses and decompress them. If the server then sends us zstd data and we're on OTP 27 we won't be able to handle it and fail the request. For this pr let's just skip setting that altogether -
Do not enable gzip compression by default. Since that is not a negotiated setting, if the replicator was talking to an older CouchDB or other server not implementing gzip decompression we'd break a customers' setup as soon as they upgrade.
-
Don't forget to fill out the template like Jan suggested
ea146fb to
f1fe550
Compare
nickva
left a comment
There was a problem hiding this comment.
Looks much better!
Added a few more comments with some minor tweaks
One new major bit I thought of is if we can make this a per job overridable. In theory we have https://docs.couchdb.org/en/stable/config/replicator.html#replicator/worker_processes to copy from (and a few others). But if that proves difficult we can punt it for later
nickva
left a comment
There was a problem hiding this comment.
Looks good but needs a few more tweaks.
We could also consider gzipping _bulk_get requests. There are not as big as _bulk_docs of course but we're bothering with _revs_diff and _bulk_get is on the order of _revs_diff so if it's good for the goose -- it's good for the gander, as they say.
nickva
left a comment
There was a problem hiding this comment.
Very nice improvements. Just added a few tiny style nits then it should be good to go
| ]}, | ||
| {ok, _} = couch_replicator_test_helper:replicate(RepObject). | ||
|
|
||
| compare_dbs(Source, Target) -> |
There was a problem hiding this comment.
Can we use cluster_compare_dbs/2 instead from the test utils module?
| -include_lib("couch/include/couch_eunit.hrl"). | ||
| -include_lib("couch/include/couch_db.hrl"). | ||
|
|
||
| -define(DOCS_COUNT, 10). |
There was a problem hiding this comment.
Since we're testing large compression bodies. At at least one test with say 500 docs
| after | ||
| config:delete("replicator", "request_compression", false), | ||
| config:delete("replicator", "compress_min_size", false) | ||
| end. |
There was a problem hiding this comment.
As a style nit, typically we don't do a lot of try...after...cleanup in tests. It's better to use proper test setup/cleanup functions. For example this pattern:
fun setup/0,
fun teardown/1,
[
?TDEF_FE(some_test)
]
setup() ->
config:set("replicator", "request_compression", "none", false),
... other set...
couch_replicator_test_helper:test_setup().
teardown(Ctx) ->
config:delete("replicator", "request_compression", false),
... other config:delete
couch_replicator_test_helper:test_teardown(Ctx).Then each test only has to set non-default values and we skip an extra indent level and an an extra 5 lines with try...after....end block.
| ; *.example.com:443:[2001:db8::1]:443 | ||
| ;connect_to = | ||
|
|
||
| ; Compress outbound replication request bodies (_bulk_docs, _revs_diff). |
There was a problem hiding this comment.
I think we forgot to mention _bulk_gets as well?
| gzip_request_body(Body, Headers) -> | ||
| Compressed = zlib:gzip(iolist_to_binary(Body)), | ||
| couch_stats:increment_counter([couch_replicator, requests_compressed, gzip]), | ||
| {Compressed, [{"Content-Length", byte_size(Compressed)}, {"Content-Encoding", "gzip"} | Headers]}. |
There was a problem hiding this comment.
Style nit: this line is getting a bit too long. Maybe break out Len = byte_size(Compressed) to shorten it a bit
Overview
Adds optional gzip compression of outbound request bodies in the replicator.
When enabled,
_bulk_docsand_revs_diffrequest bodies are gzip-compressedbefore sending. CouchDB already supports
Content-Encoding: gzipon inboundrequests so no server-side changes are needed.
Compression is disabled by default to avoid breaking setups with older or
non-CouchDB targets.
Testing recommendations
Enable compression with a low threshold and run a replication, then check the stats:
Automated tests:
couch_replicator_compression_testsinsrc/couch_replicator/test/eunit/.Related Issues or Pull Requests
Checklist
rel/overlay/etc/default.inisrc/docsfolder