Add Gzip, ZStandard, LZ4 compresssion support over memcacheClient #73
Conversation
ob1k-cache/pom.xml
Outdated
| <dependency> | ||
| <groupId>net.jpountz.lz4</groupId> | ||
| <artifactId>lz4</artifactId> | ||
| <version>1.3.0</version> |
There was a problem hiding this comment.
please move both versions to main pom dependency management
also, I'm thinking if we're doing here right that we enforce that dependency, but not a blocker for now..
| private static LZ4Factory lz4Factory = LZ4Factory.fastestInstance(); | ||
|
|
||
| public Compressor getInstance() { | ||
| switch (this) { |
There was a problem hiding this comment.
no need for the switch
just make #getInstance be abstract method and implement each one at the specific enum value
for exmaple:
LZ4 {
...getInstance() {
return new ...();
}
}
| } | ||
| } | ||
|
|
||
| private Function<byte[], byte[]> gzipDecompress() { |
There was a problem hiding this comment.
about the abstraction:
we can change Compressor to be interface, with implementation for each of the algorithms specified - removing all the impl related logic here (those methods, lz4 instance, etc)
There was a problem hiding this comment.
That's what i've originally did, but when it turned that Zstandard and LZ4 compress/decompress is a one liner, it seemed more elegant to simply provide the function.
| private final Compressor compressor; | ||
|
|
||
| public CompressionTranscoder(final Transcoder<T> delegate, Compressor compressor) { | ||
| this.delegate = Objects.requireNonNull(delegate); |
There was a problem hiding this comment.
code style thing: requireNonNull accepts also message, please add one
| @Override | ||
| public T decode(byte[] bytes) { | ||
| Objects.requireNonNull(bytes); | ||
| byte[] decompress = compressor.decompress(bytes); |
There was a problem hiding this comment.
code style thing: please add final for all defined variables (and method arguments)
| } | ||
|
|
||
| public MemcachedClientBuilder<T> withCompression() { | ||
| compressionApplier = (transcoder) -> new CompressionTranscoder<T>(transcoder, CompressionAlgorithm.LZ4.getInstance()); |
There was a problem hiding this comment.
CompressionAlgorithm can have 'default' method returning default impl, and this will just call it
| return this; | ||
| } | ||
|
|
||
| public MemcacheClientBuilder<T> build() { | ||
| if(compressionApplier != null) { |
There was a problem hiding this comment.
try the following:
private Function<Transcoder<T>, Transcoder<T>> objectSizeMonitoringApplier = Function.identity();
private Function<Transcoder<T>, Transcoder<T>> compressionApplier = Function.identity();
// setters as they now..
build() {
final Transcoder<T> wrappedTranscoder = objectSizeMonitoringApplier.compose(compressionApplier).apply(transcoder);
return wrappedTranscoder; // this also can be inlined - just for the example
}| * Created by bshushi on 26/03/2017. | ||
| */ | ||
| @RunWith(MockitoJUnitRunner.class) | ||
| public class CompressionTranscoderTest { |
There was a problem hiding this comment.
the tests we really want here is to check all compressions types as you did, using directly the compression implementation, without doing any mocking - just providing sample byte[], checking compress & de-compress works. even better - define a sample value that should be compressed, and check the size.
about the CompressionTranscoder test, it should be really thin test just checking the contract of the class - provide mocked delegator and mocked compressor and just make sure the contract is being valid.
|
LGTM |
| @Override | ||
| public byte[] compress(final byte[] data) { | ||
| if (data == null) { | ||
| throw new NullPointerException("Can't compress null"); |
There was a problem hiding this comment.
This may break existing code
| private final Compressor compressor; | ||
|
|
||
| public CompressionTranscoder(final Transcoder<T> delegate, Compressor compressor) { | ||
| this.delegate = Objects.requireNonNull(delegate, "Delegated Transcoder cannot be null."); |
There was a problem hiding this comment.
This may break existing code
|
|
||
| @Override | ||
| public T decode(byte[] bytes) { | ||
| Objects.requireNonNull(bytes, "Decompression data target cannot be null."); |
There was a problem hiding this comment.
This may break existing code
| } | ||
|
|
||
| @Test | ||
| public void testCompressionTranscoder() { |
There was a problem hiding this comment.
I'd separate this into several test cases. This will also remove the need for that somewhat almost complete implementation using mocks in the setup method
| @@ -350,6 +350,18 @@ | |||
| </dependency> | |||
|
|
|||
| <dependency> | |||
| <groupId>net.jpountz.lz4</groupId> | |||
There was a problem hiding this comment.
I'd make these guys optional, to avoid having to have these deps when you don't use them
No description provided.