Gzip compression #40

ajantis · 2017-01-18T13:25:06Z

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.

…sion setting is enabled

It is added as part of the standard ‘compress’ directive.

For some reason, Riak PUT bucket properties endpoint doesn’t handle gzipped requests properly. Given that set bucket properties requests are usually rare (compared to get/store requests) and their payload is reasonably small, it should be good enough to disable compression for them.

coveralls · 2017-01-18T13:30:06Z

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

coveralls · 2017-01-19T10:22:37Z

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

agemooij

Looks good from a functional standpoint.
I would suggest to add some tests that specifically test the compression scenarios and the known corner case with the bucket properties setup.

agemooij · 2017-01-19T10:26:00Z

src/main/resources/reference.conf

@@ -22,6 +22,9 @@ riak {
    #
    # Riak server designates values as tombstones by adding an optional 'X-Riak-Deleted' header.
    ignore-tombstones = yes
+
+    # Should the client use an compression (e.g. Gzip) when talking to Riak via HTTP.
+    enable-http-compression = yes


You might want to default this to no for backwards compatibility.

agemooij · 2017-01-19T10:28:53Z

src/main/scala/com/scalapenos/riak/internal/RiakHttpClientHelper.scala

@@ -165,7 +164,9 @@ private[riak] class RiakHttpClientHelper(system: ActorSystem) extends RiakUriSup

    val entity = JsObject("props" -> JsObject(newProperties.map(property ⇒ (property.name -> property.json)).toMap))

-    httpRequest(Put(PropertiesUri(server, bucket), entity)).map { response ⇒
+    // For some reason, Riak set bucket props HTTP endpoint doesn't handle compressed request properly.
+    // So we disable compression for this request unconditionally.


Please create an issue for this and reference it here so we can keep track of it. Perhaps also create an issue for Riak itself and link the two?

yep. Done. It's this one: #41
we have also found another interesting issue: #42

ajantis · 2017-01-19T14:07:46Z

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

coveralls · 2017-01-19T15:00:20Z

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

…guration in Spec tests

…hout compression

…it without requiring concrete tests to do anything explicitly

coveralls · 2017-01-25T14:28:02Z

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

…hen using a mixture of compression-enabled and compression-disabled clients Yes, surprisingly these tests may fail with Riak o_O

…ected gzip responses This happens when a value was stored with `Content-Encoding: gzip` and then it is fetched without requiring gzip (no `Accept-Encoding` header or with a `Accept-Encoding: identity` header). This is a reaaaally strange behaviour.

coveralls · 2017-01-26T11:35:17Z

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

coveralls · 2017-01-27T00:43:49Z

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

ajantis · 2017-01-27T01:06:11Z

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 #42 : it's in RiakGzipSpec.

coveralls · 2017-01-27T01:13:40Z

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

… handling concurrent delete operations on single values

( fixes agemooij#43 )

coveralls · 2017-01-30T14:55:54Z

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

Due to a number of Riak shortcomings in regards to handling gzipped requests, it seems safer to simply disable requests compression altogether for now.

ajantis · 2017-01-30T23:38:30Z

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...).

Apparently, that rare issue is still reproducible (see builds failures) even with clients that don’t compress requests… :-\

coveralls · 2017-01-31T00:13:38Z

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

coveralls · 2017-01-31T15:35:45Z

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

…-encoding values

coveralls · 2017-01-31T16:09:06Z

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

agemooij · 2017-02-19T14:03:09Z

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 · 2017-02-19T21:39:24Z

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 · 2017-02-20T14:03:49Z

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?

… the Readme file

ajantis · 2017-02-21T14:26:14Z

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

coveralls · 2017-02-21T14:53:04Z

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

ajantis added 8 commits January 9, 2017 00:55

Introduce a new client setting for enabling http compression

d68b872

Apply Gzip compression and decompression when the enable-http-compres…

fa22adf

…sion setting is enabled

Remove a redundant Content-Encoding header directive.

be0f01f

It is added as part of the standard ‘compress’ directive.

Replace val with def

493fec6

Introduce gzip decompression for multipart chunks

a628f90

Remove unused imports

3e5e016

Lift the multipart messages decompression onto the unmarshaller layer

8af47d9

ajantis added the enhancement label Jan 18, 2017

Remove unused import and helper method

59a8157

agemooij reviewed Jan 19, 2017

View reviewed changes

Disable gzip compression by default to be fully backwards compatible

a7a8600

ajantis added 6 commits January 21, 2017 15:30

Remove an unused dispatcher

f3b7221

Make it possible to create create an actor system with a custom confi…

0c80e69

…guration in Spec tests

Parametrize Bucket tests to verify Riak Client behaviour with and wit…

f647a94

…hout compression

tests: extract RiakClient-related tests parametrisation to a base tra…

cee5aff

…it without requiring concrete tests to do anything explicitly

tests: remove redundant whitespaces

7877c71

tests: fix IDE inspection(s)

6cf73cf

ajantis added 2 commits January 26, 2017 12:08

tests: add another set of tests for verifying compression behaviour w…

cc0e2d1

…hen using a mixture of compression-enabled and compression-disabled clients Yes, surprisingly these tests may fail with Riak o_O

ajantis mentioned this pull request Jan 27, 2017

[Compression] SetBucketProps request fails with a compressed (gzip) payload #41

Open

add a link to GitHub issue in comment for a reference

e2ca24c

ajantis mentioned this pull request Jan 27, 2017

[Compression] Riak may return a gzipped payload unexpectedly #42

Open

add a link to GitHub issue in comment for a reference

dc4053f

tests: add a test case for compression-enabled clients behaviour when…

5a53496

… handling concurrent delete operations on single values

ajantis mentioned this pull request Jan 30, 2017

[Compression] Riak inconsistent compression behaviour with 404 Not Found responses #43

Closed

Fix for the wrong 404 not found Riak responses behaviour

7c67842

( fixes agemooij#43 )

ajantis added 2 commits January 31, 2017 00:27

Remove encoding requests payload from an http request pipeline

686aa4f

Due to a number of Riak shortcomings in regards to handling gzipped requests, it seems safer to simply disable requests compression altogether for now.

Adjust a scaladoc

f7db713

Revert back the change with “safe gzip decoding”

8d5d9fe

Apparently, that rare issue is still reproducible (see builds failures) even with clients that don’t compress requests… :-\

Fix grammar in a comment

1bfb428

Add a code comment regarding handling responses with multiple content…

6401372

…-encoding values

docs: add an additional information about HTTP compression feature to…

59931b7

… the Readme file

agemooij merged commit a09fe96 into agemooij:master Feb 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gzip compression #40

Gzip compression #40

ajantis commented Jan 18, 2017 •

edited

Loading

coveralls commented Jan 18, 2017 •

edited

Loading

coveralls commented Jan 19, 2017 •

edited

Loading

agemooij left a comment

agemooij Jan 19, 2017

agemooij Jan 19, 2017

ajantis Jan 27, 2017

ajantis commented Jan 19, 2017

coveralls commented Jan 19, 2017 •

edited

Loading

coveralls commented Jan 25, 2017 •

edited

Loading

coveralls commented Jan 26, 2017 •

edited

Loading

coveralls commented Jan 27, 2017 •

edited

Loading

ajantis commented Jan 27, 2017 •

edited

Loading

coveralls commented Jan 27, 2017 •

edited

Loading

coveralls commented Jan 30, 2017 •

edited

Loading

ajantis commented Jan 30, 2017

coveralls commented Jan 31, 2017 •

edited

Loading

coveralls commented Jan 31, 2017 •

edited

Loading

coveralls commented Jan 31, 2017 •

edited

Loading

agemooij commented Feb 19, 2017

ajantis commented Feb 19, 2017

agemooij commented Feb 20, 2017

ajantis commented Feb 21, 2017

coveralls commented Feb 21, 2017 •

edited

Loading

Gzip compression #40

Gzip compression #40

Conversation

ajantis commented Jan 18, 2017 • edited Loading

Goal

Changes

Caveats

coveralls commented Jan 18, 2017 • edited Loading

coveralls commented Jan 19, 2017 • edited Loading

agemooij left a comment

Choose a reason for hiding this comment

agemooij Jan 19, 2017

Choose a reason for hiding this comment

agemooij Jan 19, 2017

Choose a reason for hiding this comment

ajantis Jan 27, 2017

Choose a reason for hiding this comment

ajantis commented Jan 19, 2017

coveralls commented Jan 19, 2017 • edited Loading

coveralls commented Jan 25, 2017 • edited Loading

coveralls commented Jan 26, 2017 • edited Loading

coveralls commented Jan 27, 2017 • edited Loading

ajantis commented Jan 27, 2017 • edited Loading

coveralls commented Jan 27, 2017 • edited Loading

coveralls commented Jan 30, 2017 • edited Loading

ajantis commented Jan 30, 2017

coveralls commented Jan 31, 2017 • edited Loading

coveralls commented Jan 31, 2017 • edited Loading

coveralls commented Jan 31, 2017 • edited Loading

agemooij commented Feb 19, 2017

ajantis commented Feb 19, 2017

agemooij commented Feb 20, 2017

ajantis commented Feb 21, 2017

coveralls commented Feb 21, 2017 • edited Loading

ajantis commented Jan 18, 2017 •

edited

Loading

coveralls commented Jan 18, 2017 •

edited

Loading

coveralls commented Jan 19, 2017 •

edited

Loading

coveralls commented Jan 19, 2017 •

edited

Loading

coveralls commented Jan 25, 2017 •

edited

Loading

coveralls commented Jan 26, 2017 •

edited

Loading

coveralls commented Jan 27, 2017 •

edited

Loading

ajantis commented Jan 27, 2017 •

edited

Loading

coveralls commented Jan 27, 2017 •

edited

Loading

coveralls commented Jan 30, 2017 •

edited

Loading

coveralls commented Jan 31, 2017 •

edited

Loading

coveralls commented Jan 31, 2017 •

edited

Loading

coveralls commented Jan 31, 2017 •

edited

Loading

coveralls commented Feb 21, 2017 •

edited

Loading