Skip to content
This repository was archived by the owner on Dec 2, 2025. It is now read-only.

Add basic LZ4 Frame v1.5.0 support - #61

Merged
odaira merged 1 commit into
lz4:masterfrom
drcrallen:lz4Frames150
May 22, 2017
Merged

Add basic LZ4 Frame v1.5.0 support#61
odaira merged 1 commit into
lz4:masterfrom
drcrallen:lz4Frames150

Conversation

@drcrallen

Copy link
Copy Markdown
Contributor

This PR is to add support for most of the features of Frames 1.5.0 as found at https://docs.google.com/document/d/1cl8N1bmkTdIpPLtnlzbBSFAdUeyNo5fwfHbHU7VRNWY

This is a port of some Kafka code

This is technically ASF Apache license 2.0 code.

@jpountz

jpountz commented Apr 25, 2015

Copy link
Copy Markdown
Collaborator

Thanks! I only skimmed through the patch but it looks great.

@jpountz

jpountz commented Apr 25, 2015

Copy link
Copy Markdown
Collaborator

Out of curiosity, do you know if the Kafka people are aware of your efforts to contribute this framing format support upstream?

@drcrallen

Copy link
Copy Markdown
Contributor Author

@jpountz Yes and no. I let the original contributor know about the patch and where to find it. But I do not have the capacity to test rolling upgrades from the prior version to this version, so I cannot immediately suggest a backport of the code to the main branch. I'm also only tangentially familiar with the way that the prior version was used in kafka in the wild.

@drcrallen

Copy link
Copy Markdown
Contributor Author

There are still a few potential issues with this PR: like the lack of capacity to specify the compressor and decompressor. But I wasn't sure what your preferred way of handling it would be.

@ederrf

ederrf commented Oct 28, 2015

Copy link
Copy Markdown

Hi @jpountz, I started using today the code provided by @drcrallen and, for my needs, it is working as expected. I'll be running more tests along the following days. Are you planning on merging this PR anytime soon ?

@jeroenvuurens

Copy link
Copy Markdown

Thanks for providing an extension. I've also started using the code provided here, it works, and the output is compatible with the unix command line lz4 util.

@hochgi

hochgi commented Feb 14, 2016

Copy link
Copy Markdown

👍 for merging this PR

@drcrallen

Copy link
Copy Markdown
Contributor Author

@jpountz anything in particular you would like to see here before merging?

@jjhart

jjhart commented Apr 1, 2016

Copy link
Copy Markdown

@drcrallen - I just made a couple comments on the pull request that might explain jpountz's hesitancy to merge this PR (not to put words in his mouth, I have no idea. I just know they were problematic when I tried to test this PR on my machine).

@drcrallen

Copy link
Copy Markdown
Contributor Author

for what its worth I fixed the mentioned items. Still not sure the best way to handle the change in lz4/lz4@f02adc7 since there's not a multiple-version-friendly flag

@FergusNelson

Copy link
Copy Markdown

I've used the LZ4FrameInputStream code to process some large (~150GB) files and can confirm that it worked very well.

@ScalaWilliam

Copy link
Copy Markdown

Would be very useful to have this. I was lost as to why I couldn't decompress a file I compressed with the reference implementation.

@odaira

odaira commented May 22, 2017

Copy link
Copy Markdown
Member

Thanks for the contribution!

I found a few problems, so I'll fix them in the next commit. I'll also revise the coding style and will add a license note.

@odaira

odaira commented Jul 14, 2017

Copy link
Copy Markdown
Member

@drcrallen Is there any reason LZ4FrameOutputStream.FrameInfo is a public class? As far as I read the code, it is used only internally in LZ4FrameOutputStream and LZ4FrameInputStream.

@ScalaWilliam

Copy link
Copy Markdown

Excellent!

@drcrallen
drcrallen deleted the lz4Frames150 branch May 7, 2018 16:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants