agemooij / riak-scala-client

An easy to use, non-blocking, fast Scala client library for interacting with Riak.
http://riak.scalapenos.com/
Apache License 2.0
84 stars 24 forks source link

Gzip compression #40

Closed ajantis closed 7 years ago

ajantis commented 7 years ago

Goal

Gzip compression allows to greatly reduce the amount of traffic flowing from/to Riak clients to/from the database. It's especially helpful when there are conflicting values ("siblings") in Riak: in most cases, they are not that much different, so we can benefit from the standard data deduplication-based compression algorithms (such as Gzip). Quick measurements demonstrated that the payload can be reduced 8-10 times with gzipping enabled.

UPDATE: Requests payload compression has been reverted and put out-of-the-scope for now. This is due to a number of known (by now) issues like #41, #42, #43... Also store object requests compression makes data stored this way in Riak incompatible with the old Riak Scala Client version (old clients won't be able to read it from Riak as it will respond with compressed responses by default, even if Accept-Encoding header has not been specified... see #42 in particular for details).

Changes

This PR introduces a toggleable gzip compression (via a flag in the reference.conf) for requests' and responses' payloads. Riak multipart responses (i.e. for 'siblings' cases) are also handled properly.

UPDATE: requests compression is put out-of-the-scope.

Caveats

The only place where it didn't work out-of-the-box is http PUT /bucket/<name>/props endpoint. For some reason, this endpoint doesn't handle gzipped payload properly. Because of that, compression has been disabled for requests targeting this endpoint.

UPDATE: this is not an issue anymore as requests compression is disabled altogether.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.3%) to 80.495% when pulling 8af47d95f5df8fcfb0a70cb606f7eb9b5fd1bc35 on ajantis:optional-http-compression into 54f742a19ab6a1bf5d40f7b4a5ef788e8d2e9138 on agemooij:master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.2%) to 81.056% when pulling 59a8157e9d5e90d95a478c85c2034eae966a8bb0 on ajantis:optional-http-compression into 54f742a19ab6a1bf5d40f7b4a5ef788e8d2e9138 on agemooij:master.

ajantis commented 7 years ago

Thanks for a swift review and feedback, Age! ;) Yep, applying it.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-2.3%) to 78.571% when pulling a7a8600cd1b6d4a7af61542f5bb70661bd4a83cc on ajantis:optional-http-compression into 54f742a19ab6a1bf5d40f7b4a5ef788e8d2e9138 on agemooij:master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.2%) to 81.056% when pulling 6cf73cfadbeef3c3953851b10435cb4c51970288 on ajantis:optional-http-compression into 54f742a19ab6a1bf5d40f7b4a5ef788e8d2e9138 on agemooij:master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.7%) to 81.505% when pulling 72cc8884cf945c2222e2410e3462338f6c2e8feb on ajantis:optional-http-compression into 54f742a19ab6a1bf5d40f7b4a5ef788e8d2e9138 on agemooij:master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.7%) to 81.505% when pulling e2ca24c2230b11aa61d0df9970f8f0a68043c77f on ajantis:optional-http-compression into 54f742a19ab6a1bf5d40f7b4a5ef788e8d2e9138 on agemooij:master.

ajantis commented 7 years ago

All RiakClient-related tests have been parametrised and are executed twice now: with and without compression. Also a few special test-cases were added to cover a workaround for issue https://github.com/agemooij/riak-scala-client/issues/42 : it's in RiakGzipSpec.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.7%) to 81.505% when pulling dc4053f333e1d66edfdb08690c70af5ae77f18e9 on ajantis:optional-http-compression into 54f742a19ab6a1bf5d40f7b4a5ef788e8d2e9138 on agemooij:master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+1.2%) to 81.988% when pulling 7c678424ec9a8c4811d494ddc0c929a080d21770 on ajantis:optional-http-compression into 54f742a19ab6a1bf5d40f7b4a5ef788e8d2e9138 on agemooij:master.

ajantis commented 7 years ago

In the end, I reverted requests payload compression for now. :-\ (see updated PR description) This causes quite some problems and makes data stored that way incompatible with old versions of riak scala client (because of #42, Riak starts returning compressed responses for that data by default...).

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.5%) to 81.366% when pulling 8d5d9fe923adcedd3df897019b50f6e997c49349 on ajantis:optional-http-compression into 54f742a19ab6a1bf5d40f7b4a5ef788e8d2e9138 on agemooij:master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.5%) to 81.366% when pulling 1bfb4289e6ef0d5ec502c5e39185ea084d176b0e on ajantis:optional-http-compression into 54f742a19ab6a1bf5d40f7b4a5ef788e8d2e9138 on agemooij:master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.5%) to 81.366% when pulling 6401372ac389912f59f11d62d104919a80e34bfe on ajantis:optional-http-compression into 54f742a19ab6a1bf5d40f7b4a5ef788e8d2e9138 on agemooij:master.

agemooij commented 7 years ago

So what is the status of this PR now? After all the problems with request compression, does this still make sense to add? If so, what are the things people should watch out for? And shouldn't we document those?

ajantis commented 7 years ago

I gave up on making request compression to work (too many brittle workarounds), so now it only works for responses. We are already using this change in prod at TT (no issues so far). So I was hoping to just contribute/merge it as it is now. =)

Sure, I can document it somewhere additionally (there are already code comments here and there + a comment for that 'compression toggle' in the reference conf file).... Readme?

agemooij commented 7 years ago

I think the only thing missing is some clear documentation on what it does and doesn't do in the README. Otherwise it might be confusing to users, right?

ajantis commented 7 years ago

@agemooij yep, agree. I added some info to README with issue references.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.5%) to 81.366% when pulling 59931b7548b2a4fed58e71433d2a2cb24ab37f86 on ajantis:optional-http-compression into 54f742a19ab6a1bf5d40f7b4a5ef788e8d2e9138 on agemooij:master.