lz4 / lz4-java

LZ4 compression for Java
Apache License 2.0
1.1k stars 253 forks source link

LZ4FrameOutputStream does not allow specifying the compressor or checksum #116

Closed LukeButters closed 6 years ago

LukeButters commented 6 years ago

Hi

It seems the LZ4FrameOutputStream and probably the corresponding input stream does not allow setting of the compressor or checksum.

I would prefer to be able to set the checksum and compressor as under windows I would like to use the pure java implementations.

I think the change would be a change to the constructor.

As a side note LZ4FrameOutputStream is marked as a final class is this needed? can you remove that to make testing easier?

-Luke

odaira commented 6 years ago

113 has been merged and it does exactly what you need. It will be included in the next release. You can use the master branch if you really need it now.

Is LZ4FrameOutputStream a final class? Many other classes in lz4-java are final, but LZ4FrameOutputStream is not, is it?

LukeButters commented 6 years ago

oh cool, thanks for that!

Yeah so https://github.com/lz4/lz4-java/blob/master/src/java/net/jpountz/lz4/LZ4FrameOutputStream.java#L43 is not marked as final while LZ4BlockOutputStream is. I prefer non final to make it easier to mock the class.

odaira commented 6 years ago

It was not me who coded LZ4BlockOutputStream, so I am not sure about its design principle, but I guess the intent of final was that the implementation of the class did not take inheritance into account.

I am open to removing final from LZ4BlockOuputStream and LZ4BlockInputStream, but can you elaborate more on why it can make testing easier, by example?

LukeButters commented 6 years ago

in general

LZ4BlockOutputStream lz4stream = org.mockito.Mockito.mock(LZ4BlockOutputStream.class)
methodThatTakesTheLz4Stream(lz4stream);
//here we can check what was done to the stream, did we close it?

mockito wont mock final classes.

I guess seeing as this is a OutputStream I can just change the type to OutputStream and maybe that is nicer.

interestingly in my code I have been doing a bunch of wrapping of OutputStream because I actually want the OS to support giving me back a input stream to read what was written. To do this I had to write an interface that happened to have all of the methods OutputStream has I can in general sub class OutputStreams to implement my interface that works well for Java's built in stream which are not final (or at least the ones I used). For the final stream what I had to do was delegate all methods to the LZ4BlockOutputStream, I do wonder if the delegate method call adds an overhead to running time?

Now that LZ4FrameOutputStream can take a compressor and it is not marked as final I can just use that. So I don't mind if LZ4BlockOutputStream stays final

odaira commented 6 years ago

Thanks for the explanation. Committed e626cc3de07443d6ea86cae9088f936070d4b552.

LukeButters commented 6 years ago

oh cool, thanks for that.