What this PR does / why we need it:

For adding compression to connections between the m3coordinator and m3aggregator.

Special notes for your reviewer: Started working on this stale PR https://github.com/m3db/m3/pull/2082, but as it has fallen behind quite a lot decided to re-implement this from scratch. Also, we needed the compression enabled for the m3msg connections instead of rawtcp.

Does this PR introduce a user-facing and/or backwards incompatible change?:

NONE

Does this PR require updating code package or user-facing documentation?:

NONE

Oct 02 '20 11:10 tsharju

All committers have signed the CLA.

Oct 02 '20 11:10 CLAassistant

Hey thanks for the contribution @tsharju - one reason why #2082 has not made further progress is that we'd like to address the issue of upgrading an existing deployment without hard downtime.

To do this supporting both snappy writers and non-snappy writers on the aggregator (server side) is required.

@arnikola has in flight changes to support this, perhaps let's look at the changes side-by-side.

Oct 06 '20 22:10 robskillington

Sure. Sounds reasonable. I suppose some kind of negotiation would be needed then to agree on the compression method used. This PR only allows configuring the m3msg consumer/producer to use snappy. Actually, we've also implemented compression for the connections between m3db and m3coordinator. There we've done it in the tchannel side where there already exists a handshake and extended that for the endpoints to agree on the compression method. That I believe would already support live upgrades.

Oct 07 '20 10:10 tsharju

Hey @tsharju here's the WIP PR to support compression with auto fallback for in-place upgrades: https://github.com/m3db/m3/pull/2731

Oct 13 '20 23:10 robskillington

Thanks @robskillington! I'd be happy to integrate that to my PR or help in any other way possible to get this feature forward. We've used the implementation in this PR in our own deployments and so far had no issues with it.

Oct 14 '20 13:10 tsharju

Codecov Report

Merging #2690 (d9fed3d) into master (d9fed3d) will not change coverage. The diff coverage is n/a.

:exclamation: Current head d9fed3d differs from pull request most recent head f8bf20d. Consider uploading reports for the commit f8bf20d to get more accurate results

@@           Coverage Diff           @@
##           master    #2690   +/-   ##
=======================================
  Coverage    72.4%    72.4%           
=======================================
  Files        1101     1101           
  Lines      102764   102764           
=======================================
  Hits        74424    74424           
  Misses      23224    23224           
  Partials     5116     5116

Flag	Coverage Δ
aggregator	`76.7% <0.0%> (ø)`
cluster	`84.9% <0.0%> (ø)`
collector	`84.3% <0.0%> (ø)`
dbnode	`78.9% <0.0%> (ø)`
m3em	`74.4% <0.0%> (ø)`
m3ninx	`73.5% <0.0%> (ø)`
metrics	`19.8% <0.0%> (ø)`
msg	`76.0% <0.0%> (ø)`
query	`67.0% <0.0%> (ø)`
x	`79.7% <0.0%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d9fed3d...f8bf20d. Read the comment docs.

Nov 13 '20 06:11 codecov[bot]

@robskillington sorry for such a late update, but I finally had more time to work on this. Made some changes to accommodate the fallback to uncompressed stream for m3msg producer/consumer.

Apr 15 '21 15:04 tsharju

[m3msg] add snappy compression for m3msg connections

Codecov Report