Allow configuration of compression codec. #26
Conversation
| } | ||
| }; | ||
|
|
||
| public abstract String getEncodingString(); |
There was a problem hiding this comment.
Can remove this method and use compressionCodec.name().toLowerCase() instead of getEncodingString(). Or even call enum constants lowercase and use just name().
There was a problem hiding this comment.
I did this because I'm anticipating that in the future we might want to add LZ4, which would manifest as an enum value LZ4 but encoding string "x-lz4"
There was a problem hiding this comment.
@gianm why not add it now?
There was a problem hiding this comment.
Since the http-client doesn't currently support LZ4 and adding it may be non-trivial work.
There was a problem hiding this comment.
@gianm ok, could you please add this explanation to getEncodingString() as a Javadoc comment?
|
@gianm this change is included into 1.0.6 release. |
|
@gianm could you please review some of my recent PRs into Druid? |
|
sure, I have been doing more development than reviewing this week, but was intending to get back to reviewing soon. |
|
thank you for your review. |
|
@gianm could you please describe a bit how this code change controls the compression? As I understand the ACCEPT_ENCODING header means which content encoding the client is able to decode. So it can disable the response compression, but not the request compression. |
|
And where the compression logic is? Is it in Druid? |
In Druid we've seen that for large resultsets, gzip compression / decompression time is substantial, and given that Druid is typically deployed on a LAN we could benefit from using a different codec or from not using compression at all. This patch adds configurability between gzip/deflate/identity (the ones currently supported by the client) rather than hard-coding gzip.
Allows changing the default through
withCompressionCodecon the config builder, as well as overriding per-request through the Accept-Encoding header.