Support for handling compressed responses in the HTTP connector

[marcelog]

Hey guys,

This patch tries to add support for handling incoming compressed content in the http connector. The general idea is to read the chunks of the reponse in buffers instead of concatenating them as a string (so we can support binary data), and then uncompress it if the Content-Encoding header is present and matches gzip.

The pull includes two unit tests, both pass. The first one will test the positive case, where the transport detects, uncompress, and returns a response. The second one will test the negative case where uncompressing the data returns an error (in this case, the error and the original response are returned).

Please take into account that I'm not that experienced in writing unit tests for nodejs and using these frameworks in general, but the code seems reasonable.

The patch has been also tested manually with ElasticSearch 1.3.2, setting:

http.compression: true
http.compression_level: 9

And instantiating the client with:

{
  hosts: [
    {
      host: 'localhost',
      headers: {
        'Accept-Encoding': 'gzip, deflate'
      }
    }
  ]
}

Any thoughts? Thanks in advance.

[spalger]

Why are you using zlib.unzip() instead of zlib.gunzip()?

I feel like there is a disconnect between the header you specified (Accept-Encoding: gzip, deflate) and the condition here. I think that there should be a check for deflate somewhere and that the zlib streams should be used instead of the async methods that wait for the end of the response.

Unfortunately, this is mostly just a guess (based on naming) because the node docs are pretty pitiful for those methods.

[spalger]

Awesome request! I placed some comments in-line, if you could respond inline as well I would appreciate it.

[marcelog]

@spenceralger (replying to your comment about the use of unzip/gunzip that my commit wiped out, sorry):

I chose to use zlib.unzip() because it ends up creating an Unzip class. The nodejs doc for the Unzip class states that it will "Decompress either a Gzip- or Deflate-compressed stream by auto-detecting the header.". So I figured that would be the "safest bet". I agree on checking for the deflate algorithm, so I updated the code to include the check, and a unit test, so we test gzip and deflate in separate cases.

[spalger]

Great!

[coveralls]

Coverage increased (+0.02%) when pulling 83b6400 on marcelog:marcelog_gzip_content into 01d233a on elasticsearch:2.4.

[marcelog]

@spenceralger Replying to 2 of your comments. I added a unit test, so we can test both gzip and deflate algorithms in separate cases (your comment was about setting content-encoding to gzip and using deflate to compress the data).

Your other comment was about testing it with a larger payload, so I created a small loop to send a lot of text, they are actually small json objects serialized into a string. Do you think it does the job?

[spalger]

I think that should work great

[spalger]

Thanks again @marcelog! Merged into master, will release 2.4.2 later today with an option to enable compression globally.

🍻

[marcelog]

Thank you @spenceralger! :) Btw, I thought about adding it as an option to the client, but then I thought that this would imply that the client should send the requests in a compressed form too (or perhaps not all of the requests, and that would imply an option for the requests instead of the client). So I opted for the "hacky" way, that means making the client aware of compressed responses and one having to specify an extra header in the host configuration and not supporting sending compressed requests.

How will the new option work? Can't wait for it! :)

[spalger]

I'm thinking that users would need to set suggestCompression: true. This would be accompanied by a note explaining that "enabling this setting tells elasticsearch, on each request, that it can send compressed responses to this client".

var client = new es.Client({
  host: 'localhost:9200',
  suggestCompression: true
});

The "compression detection" logic will be in place regardless, so setting this per host (via the headers as you did before) will still work.

[spalger]

On an important side note: have you signed the CLA?

[marcelog]

Oh I see, cool :)

Yes, I signed it yesterday, or the day before. I can send the pdf I got back if needed.